cloudfoundry / dotnet-core-buildpack

Cloud Foundry buildpack for .NET Core on Linux
http://docs.cloudfoundry.org/buildpacks/
Apache License 2.0
91 stars 90 forks source link

Building from source uses highly flawed method of detecting versions #520

Open macsux opened 2 years ago

macsux commented 2 years ago

Current buildpack tries to parse .csproj file as XML to extract TargetFramework and a few other properties to determine the correct version to introduce. This is a highly flawed way of doing it because .csproj is essentially an MSBuild script. There can be a very large number of ways to set these properties in a way that is non standard for all sorts of legit reasons. The value of <TargetFramework> may actually be a variable that comes from a common import file at parent level. It may be set by Directory.Build.props which is a common way to set it for multiple projects simultaneously as it's automatically imported from a parent folder. It may be absent altogether and declared as signifying that the project can run on multiple runtimes. The only thing that can accurately answer which TargetFramework is actually being used is MSBuild itself.

The simplest way to query MSBuild for this info is to inject a target that echos the property to console and parse it. For example if you append this to the bottom of csproj

  <Target Name="__GetFrameworkVersion">
    <Message Text="TargetFramework is '$(TargetFramework)'" Importance="High" />
  </Target>

one can print out the value of TargetFramework to console by running

> dotnet msbuild /t:__GetFrameworkVersion

Microsoft (R) Build Engine version 17.0.0+c9eb9dd64 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  TargetFramework is 'net6.0'
arjun024 commented 2 years ago

@ForestEckhardt @fg-j @sophiewigmore Hi! I'm wondering if you all have any opinions/thoughts about this issue given your experience with dotnet+buildpacks, and since we use identical logic (link, link) in the paketo dotnet-core buildpack?

fg-j commented 2 years ago

I'd love to determine the target version in a more idiomatic way. I don't think we can directly use the proposal outlined here in the CNBs, though, because we don't have access to the dotnet CLI in the detect phase of the build.

macsux commented 2 years ago

It's not ideal, but this might be a case for a new "advanced fat builder" that includes key tooling from each ecosystem available to buildpacks (latest .net core SDK, maven/gradle). The downside is obviously bloated base image size. I imagine Java has similar issue given that a java is selected by default from spring parent pom file, but can totally be overridden via something like this in pom file.

<properties>
    <java.version>9</java.version>
</properties>

And that value doesn't have to be a constant - might a variable declared earlier or in parent POM. Gradle gets even more flexible in its configuration and the only right way to get an answer is to ask the underlying tooling.

Fat builder would also allow buildpacks to be written in .NET which was actually popular alternative to writing them in Go in Cloud Foundry ecosystem. There's a whole bunch of buildpacks that were spawned for CF written on top of my .NET Buildpack.V2 template - first 2 now being owned by TAS R&D.

johnnyr0x commented 1 year ago

Considering closing this as users can define the TargetFramework and a fat builder doesn't align with our current strategy.

sophiewigmore commented 1 month ago

@macsux I'm re-investigating this. Since the "fat builder" concept isn't viable still, I was wondering if you think parsing the Directory.Build.props file similarly to how we parse the .csproj file is still problematic? From your original issue, I gathered that checking the .csproj was problematic because there were a myriad of other ways the version might be set. It seems like since using a Directory.Build.props is a more common way to set the version, that parsing the file might be alright?

macsux commented 1 month ago

This would tackle just one specific scenario and will continue to remain brittle. Here are scenarios where it wouldn't work:

macsux commented 1 month ago

Instead of adding stuff to the builder, is there a reason why dotnet buildpacks cannot bundle and use MSBuild during detect phase? For example, Jetbrains free MSBuild redistributable is only 13mb, and can probably be slimmed down further as I doubt everything in it is required to do what we're trying to do here - just MSBuild "core" engine. https://blog.jetbrains.com/dotnet/2018/04/13/introducing-jetbrains-redistributable-msbuild/

sophiewigmore commented 1 month ago

@macsux thank you for weighing in, that helps my understanding. Adding MSBuild to detect could be possible, but its not a path we've ever explored during this phase as far as I know. I'd be more open to adding that if we got some feedback from customers that this is something they'd really need, as this issue has been open for 2 years with little action, even though its an official .NET use case. The users I know who are interested in this really only care about the TargetFramework. Though brittle, it just might not be worth the tradeoff to support the wider use case

macsux commented 1 month ago

BTW, I'll highlight that on TAS (v2), you already have dotnet sdk inside buildpack and what I'm proposing can be done for cloud foundry specific dotnet buildpack. For V3 (CNBs), this is imo a self inflicted wound brought on by the fact that dotnet buildpack is made of 7 mini-buildpacks. If they were single one, you would also have access to MSBuild during detect phase (as it's part of the buildpack packaging and can be utilized by detect iself).