Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
macsux opened this issue Feb 28, 2022 · 9 comments
Open

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

macsux opened this issue Feb 28, 2022 · 9 comments

Comments

@macsux
Copy link

macsux commented Feb 28, 2022

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
Copy link
Member

@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
Copy link
Member

fg-j commented Apr 14, 2022

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
Copy link
Author

macsux commented Apr 19, 2022

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
Copy link

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

@sophiewigmore
Copy link
Member

@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
Copy link
Author

macsux commented Jul 25, 2024

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

  • Directory.Build.props may appear at multiple levels along the tree hierarchy. At each levle, this file may CHOOSE to include the one above it, but by default it does not. For example it may include a line like this to continue including parent ones, or if it doesn't have it then nothing above it gets processed: <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
  • Directory.Build.props is just a convention based "import" of another msbuild fragment file. Developer may choose to import common fragments explicitly, like steeltoe does
  • TargetFramework is essentially a variable in MSBuild engine. You're assuming its value is constant "somewhere". But its value may be derived from other variables. A developer may choose to have custom logic on selection of TargetFramework, and have fragments of this spread all over msbuild files. Example, which is a completely valid MSBuild pattern - one I've used in my own projects:
  <PropertyGroup>
    <AllowPrerelease>True</AllowPrerelease>
    <StableTargetFramework>net8.0</StableTargetFramework>
    <PrereleaseTargetFramework>net9.0</PrereleaseTargetFramework>
    <TargetFramework Condition="$(AllowPrerelease) == 'True'">$(PrereleaseTargetFramework)</TargetFramework>
    <TargetFramework Condition="$(AllowPrerelease) == 'False'">$(StableTargetFramework)</TargetFramework>
  </PropertyGroup>

@macsux
Copy link
Author

macsux commented Jul 25, 2024

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
Copy link
Member

sophiewigmore commented Aug 2, 2024

@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
Copy link
Author

macsux commented Aug 8, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Not scoped
Development

No branches or pull requests

6 participants