-
Notifications
You must be signed in to change notification settings - Fork 13
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 changes for .NET 9 RPMs #345
Conversation
I built .NET 9 Preview 1 locally and ran tests against it. Here are some obvious fixes.
release_prefix = '.'.join(parts[1].split('.')[0:2]) | ||
return parts[0] + '~' + release_prefix | ||
# Convert upstream version 11.2.3-preview.999.1234 to RPM-style 11.2.3~preview.999.1234 | ||
return version.replace('-', '~') |
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.
RPMs previously used preview.1
instead of preview.1.24080.9
mostly because it was difficult to get accurate information before building things. With release.json, this is easy to fix, so I fixed it.
@@ -5,7 +5,7 @@ IFS=$'\n\t' | |||
set -x | |||
|
|||
sdk_version="$(dotnet --version)" | |||
runtime_version="$(dotnet --list-runtimes | head -1 | awk '{ print $2 }')" | |||
runtime_version="$(dotnet --list-runtimes | grep Microsoft.NETCore.App | awk '{ print $2 }')" |
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.
dotnet --list-runtimes
lists all the runtimes, including ASP.NET Core. This filter was relying on a certain ordering that doesn't work with .NET 9 Preview 1.
@@ -20,6 +20,6 @@ else | |||
fi | |||
|
|||
framework_dir=$(../dotnet-directory --framework "$1") | |||
dotnet tool update -g dotnet-ef --version "${framework_dir#*App/}.*-*" | |||
dotnet tool update -g dotnet-ef --version "${framework_dir#*App/}.*" |
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 am not sure why we previously used *-*
. Will this change have a negative impact? Thoughts, @tmds ?
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.
With a preview version, this was evaluated as 9.0.0-preview.1.24080.9*-*
, which doesn't match anything. But 9.0.0-preview.1.24080.9*
does.
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.
@nicrowe00 updated this test since it was failing for .NET 8.0.1 here: #333.
This test is skipped in our CI, so we haven't noticed it doesn't work against preview versions.
With the March release (8.0.3
), we should be able to go back to how this was before #333 because dotnet-ef
should then run on any 8.0 runtime version.
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 I merge this change, then, or defer it to a separate PR (or never)?
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.
@nicrowe00 can you remind us why we had to use *-*
here?
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.
can you remind us why we had to use - here?
He didn't add it. It was already there.
Before #333 we prefixed this only with the major version number. So the --version
argument was for example 7.*-*
. This means the SDK it is allowed to install a version higher than 7. It will prefer to install a stable version (e.g. 7.0.0
) but if there is no stable version, it can install a preview (e.g. 7.0.0-preview1
).
With #333, if you run against a released version of .NET, you get something like 8.0.1.*-*
. For the SDK this is an acceptable version pattern.
When #333 runs against a .NET 9 preview, the --version
becomes something like 9.0.0-preview.1.24080.9*-*
and this pattern doesn't make sense to the SDK.
This PR is now changing that version into 9.0.0-preview.1.24080.9.*
. And if this is working, it seems the SDK treats this as 9.0.0-*
(and a suffix of at least preview.1.24080.9
?) since the published dotnet-ef
version is actually 9.0.0-preview.1.24081.2
.
Should I merge this change, then, or defer it to a separate PR (or never)?
I'm fine with either what you have here, or the pattern we had before #333 (which will work for .NET 9 previews, and should start working for .NET 8 on the next patch release).
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.
He didn't add it. It was already there.
I was thinking of #319.
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.
Because I suggested it in that PR for the reasons mentioned above.
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 is the documentation for the wildcard pattern: https://github.com/NuGet/Home/wiki/Support-pre-release-packages-with-floating-versions.
I built .NET 9 Preview 1 locally and ran tests against it. Here are some obvious fixes.