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

Initial implementation #2

Merged
merged 10 commits into from
Sep 27, 2022
Merged

Initial implementation #2

merged 10 commits into from
Sep 27, 2022

Conversation

ForestEckhardt
Copy link
Contributor

Resolves #1

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

@ForestEckhardt ForestEckhardt marked this pull request as draft September 14, 2022 20:34
buildpack.toml Outdated Show resolved Hide resolved
@ForestEckhardt ForestEckhardt marked this pull request as ready for review September 15, 2022 15:54
detect.go Outdated Show resolved Hide resolved
@sophiewigmore
Copy link
Member

@ForestEckhardt its been a bit so I forgot some context- could you remind me why it's preferable to check the runtimeconfig.json for existing and for runtime versions during build, rather than using its existence as detection criteria?

@ForestEckhardt
Copy link
Contributor Author

@sophiewigmore We check it at build because this buildpack will now run after dotnet-publish and we want to pull the version of the framework from the runtimeconfig if at all possible with is generated during publish. Therefore we have to check after the during build.

@sophiewigmore
Copy link
Member

Ahhh yes, thank you

Copy link

@fg-j fg-j left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks, but a couple of key questions:

  1. the name of the buildpack
  2. the priority order of version resolution

buildpack.toml Outdated Show resolved Hide resolved
buildpack.toml Outdated Show resolved Hide resolved
buildpack.toml Outdated Show resolved Hide resolved
buildpack.toml Outdated Show resolved Hide resolved
buildpack.toml Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
runtime_config_parser.go Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
integration/layer_reuse_test.go Outdated Show resolved Hide resolved
ForestEckhardt and others added 3 commits September 26, 2022 09:35
Co-authored-by: Frankie G-J <frankieg@vmware.com>
Co-authored-by: Frankie G-J <frankieg@vmware.com>
instead of indexing into image.Buildpacks array
buildpack.toml Outdated
name = "ASP.NET Core Runtime"
licenses = ["MIT", "MIT-0"]
purl = "pkg:generic/dotnet-aspnetcore@3.1.28?checksum=a3fe5848d88dc67cbfceaf0b8e6d3e1178f8441bfedbb21861b2e72fdee3f28d&download_url=https://download.visualstudio.microsoft.com/download/pr/effaa5bf-0fa7-4e5a-9ce8-9ac04ee86669/5afb2b1c2ad68550cec914d8fb303d20/aspnetcore-runtime-3.1.28-linux-x64.tar.gz"
sha256 = "a3fe5848d88dc67cbfceaf0b8e6d3e1178f8441bfedbb21861b2e72fdee3f28d"
Copy link

Choose a reason for hiding this comment

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

should we switch to using checksum + the published SHA512 now that it's possible in jam and packit?

Copy link

Choose a reason for hiding this comment

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

or do you want to defer that to a subsequent PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can take a quick crack at it.

@fg-j fg-j self-requested a review September 26, 2022 20:04
Copy link

@fg-j fg-j left a comment

Choose a reason for hiding this comment

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

@ForestEckhardt I just experimented with combining this buildpack with the existing rearchitecture spike, and I'm seeing the running app fail to locate the ASP.NET Core runtime, even though the runtime is present in a layer on the app image. The app fails to start with:

docker run -it --rm --env PORT=8080 test-rearch
Setting ASPNETCORE_URLS=http://0.0.0.0:8080
You must install .NET to run this application.

App: /workspace/source-app
Architecture: x64
App host version: 6.0.9
.NET location: Not found

Learn about runtime installation:
https://aka.ms/dotnet/app-launch-failed

Download the .NET runtime:
https://aka.ms/dotnet-core-applaunch?missing_runtime=true&arch=x64&rid=ubuntu.18.04-x64&apphost_version=6.0.9

However, running

DOTNET_ROOT=/layers/paketo-buildpacks_dotnet-core-aspnet-runtime/dotnet-core-aspnet-runtime /workspace/source-app

inside the container makes the app start as expected. I believe this buildpack must set DOTNET_ROOT.

@ForestEckhardt
Copy link
Contributor Author

@fg-j After our short investigation is appears that the DOTNET_ROOT needs to be set for FDEs I will make a commit shortly.

@sophiewigmore
Copy link
Member

@ForestEckhardt @fg-j I'm surprised that DOTNET_ROOT needs to be set if the runtime is already on the PATH. Could you give a bit more context about why that is Forest?

@ForestEckhardt
Copy link
Contributor Author

@sophiewigmore It have to do with how FDE works. Because we are not invoking the dotnet cli the FDE is like trying to look in a default install location which we have not installed the installation in. Because of that the warning message given tells us we need to specify the DOTNET_ROOT if we are in the non-default location.

However because of our restructure we can get away with using the default directive instead of the override directive which is the preferred option.

@sophiewigmore
Copy link
Member

@ForestEckhardt thank you! that makes sense

@sophiewigmore
Copy link
Member

@fg-j are you able to re-test with the changes here since you have the re-architecture spike pulled already? Assuming that works now, this PR looks good to me

@fg-j fg-j self-requested a review September 27, 2022 16:19
Copy link

@fg-j fg-j left a comment

Choose a reason for hiding this comment

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

I've verified that with the new changes setting DOTNET_ROOT, the buildpack now integrates as expected with the other language family buildpacks. I've built and run source apps, FDDs, FDEs, and self-contained apps successfully.

@fg-j fg-j merged commit 57115d5 into main Sep 27, 2022
@fg-j fg-j deleted the initial-implementation branch September 27, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement .NET RFC 0005
3 participants