-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update samples to .NET 8 #4742
Update samples to .NET 8 #4742
Conversation
Publishing these samples will be blocked on dotnet/docker-tools#1159 |
Same thing for #4743 |
Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com>
@richlander, I'll be trying a few things here to get these samples building. |
I ran into this issue trying to compile with the nightly sdk aot rc
|
@@ -36,7 +36,7 @@ steps: | |||
condition: and(succeeded(), ${{ parameters.condition }}) | |||
- script: > | |||
echo "##vso[task.setvariable variable=runImageBuilderCmd] | |||
docker run --rm | |||
\$env:DOCKER_BUILDKIT=1; docker run --rm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed anymore.
@@ -1,5 +1,5 @@ | |||
variables: | |||
imageNames.imageBuilderName: mcr.microsoft.com/dotnet-buildtools/image-builder:2230260 | |||
imageNames.imageBuilderName: mcr.microsoft.com/dotnet-buildtools/image-builder:2233114 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imageNames.imageBuilderName: mcr.microsoft.com/dotnet-buildtools/image-builder:2233114 | |
imageNames.imageBuilderName: mcr.microsoft.com/dotnet-buildtools/image-builder:2240150 |
samples/aspnetapp/Dockerfile
Outdated
@@ -1,19 +1,21 @@ | |||
# Learn about building .NET container images: | |||
# https://github.com/dotnet/dotnet-docker/blob/main/samples/README.md | |||
FROM mcr.microsoft.com/dotnet/sdk:7.0 AS build | |||
FROM --platform=$BUILDPLATFORM mcr.microsoft.com/dotnet/sdk:8.0-preview AS build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 8.0-preview
tags in this PR will need to be updated to just 8.0
.
|
||
# copy and publish app and libraries | ||
COPY . . | ||
RUN dotnet publish -a $TARGETARCH --self-contained --no-restore -o /app releasesapi.csproj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use --ucr
instead of --use-current-runtime
then we should use --sc
instead of --self-contained
. Applies to other Dockerfiles too.
## Supported Linux distros | ||
|
||
The .NET Team publishes images for [multiple distros](../../documentation/supported-platforms.md). | ||
|
||
Samples are provided for: | ||
|
||
- [Alpine](Dockerfile.alpine) | ||
- [Alpine with ICU installed](Dockerfile.alpine-icu) | ||
- [Debian](Dockerfile.debian) | ||
- [Ubuntu](Dockerfile.ubuntu) | ||
- [Ubuntu Chiseled](Dockerfile.chiseled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have Dockerfile.chiseled-icu that uses the extra
image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Let's have a conversation on how to handle ICU with these samples. It's a question of whether we want to favor a little extra ceremony or push people to SCD (which the extra images would required). I'm open to either, but its a policy topic for us to discuss.
{ | ||
// Only show releases in support or < 1 year EOL | ||
if (DateOnly.TryParse(releaseSummary.EolDate, out DateOnly eolDate) | ||
&& DateOnly.FromDateTime(DateTime.UtcNow.AddYears(-1)).DayNumber > eolDate.DayNumber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& DateOnly.FromDateTime(DateTime.UtcNow.AddYears(-1)).DayNumber > eolDate.DayNumber) | |
&& DateOnly.FromDateTime(DateTime.UtcNow.AddYears(-1)).DayNumber > eolDate.DayNumber) |
{ | ||
HttpClient httpClient = new(); | ||
var loadError = "Failed to load release information."; | ||
var releases = await httpClient.GetFromJsonAsync<ReleaseIndex>(ReleaseValues.RELEASE_INDEX_URL, ReleaseJsonSerializerContext.Default.ReleaseIndex) ?? throw new Exception(loadError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that the library from https://www.nuget.org/packages/Microsoft.Deployment.DotNet.Releases isn't used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this sample for another reason, to demonstrate System.Text.Json (for a blog post). We can certainly update this code to use that library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider deleting this. There's only one project here and this doesn't add any value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's devkit. It generates these all the time so I decided to check it in.
// if (os == Tests.OS.Alpine) | ||
// { | ||
// os += "-slim"; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete
// new SampleImageData { OS = OS.Alpine, Arch = Arch.Amd64, DockerfileSuffix = "alpine-slim", IsPublished = true }, | ||
// new SampleImageData { OS = OS.Alpine, Arch = Arch.Arm, DockerfileSuffix = "alpine-slim", IsPublished = true }, | ||
// new SampleImageData { OS = OS.Alpine, Arch = Arch.Arm64, DockerfileSuffix = "alpine-slim", IsPublished = true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to be replaced with the jammy-chiseled Dockerfiles?
The dotnet-docker/tests/Microsoft.DotNet.Docker.Tests/TestAppArtifacts/NuGet.config.nightly Line 7 in f270639
|
Co-authored-by: Matt Thalman <mthalman@microsoft.com>
Looks like there is a NanoServer issue with virtualization/Docker... |
What should we do about the Nanoserver issue? I assume that doesn't block merging then? |
You'll notice the build was only failing due to the ComplexAppTest on NanoServer legs.. A few notes on that:
Hopefully the changes make it work, and the strange error message was just a side effect of things being different between Docker Desktop/the build agents with Docker CE. |
…test" This reverts commit b44093a.
@@ -92,28 +92,22 @@ public async Task VerifyAspnetSample(SampleImageData imageData) | |||
[Fact] | |||
public void VerifyComplexAppSample() | |||
{ | |||
// complexapp sample doesn't currently support building on Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong. The app works fine on Windows, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app works fine but the Dockerfile is multi-platform so it won't work on Windows. I can amend the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge first and then deal with that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #4873 for this.
Co-authored-by: Justin Anderson <jander-msft@users.noreply.github.com> Co-authored-by: Logan Bussell <loganbussell@microsoft.com> Co-authored-by: Damian Edwards <damian@damianedwards.com> Co-authored-by: Matt Thalman <mthalman@microsoft.com>
The samples need to be updated to .NET 8. We intend to publish these changes with .NET 8 RC1.
Intended changes:
8080
-a
)--platform $BUILDPLATFORM
)PublishTrimmed
-- should be in project files so that developers get good feedback from analyzers. Our previous use of msbuild properties in Dockerfiles is an anti-pattern.slim
versions because they rely on project properties in Dockerfiles and due to MVC apps no longer work with PublishTrimmed aspnetcore#49360.TODO:
docker buildx build
Reported two bugs while doing this work:
PublishAot
should have better error messages when cross-compiling runtime#88942-a
sets app to framework-dependent whenSelfContained
istrue
sdk#34026Related: