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.

Sign up for GitHub

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

Jump to bottom

Use Microsoft SDK Hive directly #366

Merged
merged 5 commits into from
Jul 14, 2022
Merged

Use Microsoft SDK Hive directly #366

merged 5 commits into from
Jul 14, 2022

Conversation

sophiewigmore
Copy link
Member

@sophiewigmore sophiewigmore commented Jul 7, 2022 •
edited
Loading

Summary

Part of paketo-buildpacks/dotnet-core#722

Use the Microsoft official SDK download, which includes the runtime and libraries, instead of the Paketo-hosted, stripped down version

** Note ** the URI and SHA are hardcoded. A separate PR to update the dep-server metadata will go in after this PR is merged for future versions.

Other changes:

  • URI and SHA in buildpack.toml are the same as the source URI and SHA
  • Remove SDK compatibility tables
  • Limit number of patches per version line to 1, since SDK Hive is compatible with any runtime/asp.net in the same major/minor version line.
  • No longer require dotnet-runtime, as it's included in the SDK
  • Look at BP_DOTNET_FRAMEWORK_VERSION as a version source instead of RUNTIME_VERSION
  • Consider *.*proj files as a version source
  • Remove DOTNET_ROOT symlinking in favour of copying the .NET CLI into the /workspace/.dotnet_root and making it available on the path at launch-time. This is necessary for the FDD case, which will need access to the CLI at launch.

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@sophiewigmore sophiewigmore requested a review from a team as a code owner July 7, 2022 20:11
@sophiewigmore sophiewigmore marked this pull request as draft July 7, 2022 20:11
@sophiewigmore sophiewigmore self-assigned this Jul 7, 2022
@sophiewigmore sophiewigmore added the semver:minor A change requiring a minor version bump label Jul 7, 2022
@sophiewigmore sophiewigmore marked this pull request as ready for review July 8, 2022 13:25
@sophiewigmore
Copy link
Member Author

@paketo-buildpacks/dotnet-core-maintainers I'll go in and update the dep-server metadata for the versions in this iteration of the buildpack once this is approved. However, I'm wondering if it's at all problematic to swap over versions to the Microsoft versions if we've already shipped buildpacks with that version?

Use Microsoft SDK Hive directly
b5dea75 
- URI and SHA in buildpack.toml are the same as the source URI and SHA
- No longer `require` dotnet-runtime
- Look at BP_DOTNET_FRAMEWORK_VERSION as a version source instead of
RUNTIME_VERSION
- Consider `*.*proj` files as a version source
- Remove `DOTNET_ROOT` symlinking
@fg-j
Copy link

fg-j commented Jul 13, 2022

I think that's probably fine. Maybe we call this a minor version bump instead of a patch?

@sophiewigmore
Copy link
Member Author

sophiewigmore commented Jul 13, 2022 •
edited
Loading

@fg-j there's already a minor bump label on here!

@fg-j
Copy link

fg-j commented Jul 13, 2022

@sophiewigmore did you happen to test run these changes against paketo-buildpacks/dotnet-core#703 or paketo-buildpacks/dotnet-core#670?

@sophiewigmore
Copy link
Member Author

sophiewigmore commented Jul 13, 2022 •
edited
Loading

@fg-j Yes, I saw a successful build of https://github.com/blogifierdotnet/Blogifier with:

 pack build blogifier -e BP_DOTNET_PROJECT_PATH="src/Blogifier" --env BP_DOTNET_PUBLISH_FLAGS="--self-contained=true" -b <path to local .NET Core language family buildpackage> -v --clear-cache

I was able to curl the page and see the Blogifier app.
And I also saw a successful build of https://github.com/ManuDinicola/NetPublishedFailed with:

pack build testpublish --env BP_DOTNET_PROJECT_PATH=./src/WebApi --env BP_DOTNET_PUBLISH_FLAGS="--verbosity=detailed" -b <path to local .NET Core language family buildpackage>

I'd love for someone else to try it out as well if they have the time. It requires the sw-patch-1 version of SDK, Runtime, ASP.NET, and Publish buildpacks in the language family though, so if it's too much of a pain to test out that's fine too.

@fg-j
Copy link

fg-j commented Jul 13, 2022

@sophiewigmore Since you've taken it for a spin, I'll probably wait to do that as an acceptance step at the language family level before we cut a release.

fg-j
fg-j reviewed Jul 13, 2022
View reviewed changes
build.go
Comment on lines +104 to +106
sdkLayer.BuildEnv.Prepend("PATH", sdkLayer.Path, string(os.PathListSeparator))
logger.EnvironmentVariables(sdkLayer)
envLayer.LaunchEnv.Prepend("PATH", filepath.Join(context.WorkingDir, ".dotnet_root"), string(os.PathListSeparator))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setting one in the BuildEnv and the other in the LaunchEnv to prevent a case where both layers put a version of the dotnet CLI on the PATH?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite, it's more to prevent more than what's needed being available during launch. The dotnet CLI in both cases is the same. This is confusing and I would've liked to find a way to avoid doing this. The reason for this code is:

  1. The first case is in the .NET Publish case, we want the full SDK hive, so we make the entire SDK layer available on the PATH during build.
  2. The less obvious case is in the .NET Execute case (FDD case specifically), the dotnet CLI is needed at launch-time, so we just copy it into the /workspace/.dotnet_root to isolate it, and make it accessible via the PATH. This envLayer is ALWAYS marked for launch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can think of a nicer way to just have the dotnet CLI available at launch-time, let me know, I'm all ears.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for laying this out. That makes sense to me. I agree it's not the nicest, but I'm ok with it for now because Forest + my forthcoming proposal to expand the ASP.NET dependency + rearrange buildpack order will enable us to use the dotnet CLI that ships with ASP.NET at launch time in the future!

@fg-j fg-j self-requested a review July 14, 2022 15:21
@fg-j fg-j merged commit 833e703 into main Jul 14, 2022
@fg-j fg-j deleted the sw-patch-1 branch July 14, 2022 16:20
@fg-j fg-j mentioned this pull request Jul 15, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fg-j fg-j fg-j approved these changes

@ForestEckhardt ForestEckhardt Awaiting requested review from ForestEckhardt ForestEckhardt is a code owner automatically assigned from paketo-buildpacks/dotnet-core-maintainers

Assignees

@sophiewigmore sophiewigmore

Labels
semver:minor A change requiring a minor version bump
Projects
Paketo Workstreams
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sophiewigmore @fg-j