dotnet: august 2024 upgrades #335969
Merged
dotnet: august 2024 upgrades #335969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
|
Result of 13 packages failed to build:
75 packages built:
|
corngood
force-pushed
the
dotnet-upgrade
branch
from
4b6e6cf
to
2ac5f6a
Compare
ofborg
bot
added
8.has: package (new)
|
Result of 3 packages failed to build:
132 packages built:
|
corngood
force-pushed
the
dotnet-upgrade
branch
from
2ac5f6a
to
39db502
Compare
ofborg
bot
added
the
11.by: package-maintainer
This fixes intermittent ArgumentOutOfRangeExceptions when building certain packages (e.g. roslyn-ls).
|
Result of 2 packages failed to build:
157 packages built:
|
This is an ongoing problem: #325857
This is also broken in master, but I'm not 100% sure why |
konradmalik
reviewed
Aug 22, 2024
| @@ -1,9 +1,9 @@ | |||
| { lib, fetchFromGitHub, buildDotnetModule, dotnetCorePackages, stdenvNoCC, testers, roslyn-ls, jq }: | |||
| let | |||
| pname = "roslyn-ls"; | |||
| dotnet-sdk = dotnetCorePackages.sdk_9_0; | |||
| dotnet-sdk = with dotnetCorePackages; combinePackages [ sdk_6_0 sdk_7_0 sdk_8_0 sdk_9_0 ]; | |||
There was a problem hiding this comment.
perhaps this is my lack of understanding, but why do we add those other SDKs back here?
In the last PR for this package I've removed them which did not change deps.nix at all.
Also there is #326335 which requires to stop using sdk_7_0.
There was a problem hiding this comment.
roslyn-ls uses the runtime packages from dotnet 6, 7, 8, and 9. Including 7 here removes the dependency on its packages. Unfortunately 6 and 8 are still present using older versions (6.0.32 and 8.0.7).
I think this is because sdk 9 preview 7 has hardcoded references to 6.0.32, 7.0.20, and 8.0.7, but I haven't had a chance to go digging for that yet.
I'm not sure what the best solution to this is.
There was a problem hiding this comment.
This might be an option:
diff --git a/pkgs/by-name/ro/roslyn-ls/package.nix b/pkgs/by-name/ro/roslyn-ls/package.nix
index 3207eea29d09..5f5096f28f4d 100644
--- a/pkgs/by-name/ro/roslyn-ls/package.nix
+++ b/pkgs/by-name/ro/roslyn-ls/package.nix
@@ -1,9 +1,9 @@
{ lib, fetchFromGitHub, buildDotnetModule, dotnetCorePackages, stdenvNoCC, testers, roslyn-ls, jq }:
let
pname = "roslyn-ls";
- dotnet-sdk = with dotnetCorePackages; combinePackages [ sdk_6_0 sdk_7_0 sdk_8_0 sdk_9_0 ];
+ dotnet-sdk = dotnetCorePackages.sdk_9_0;
# need sdk on runtime as well
- dotnet-runtime = dotnetCorePackages.sdk_9_0;
+ dotnet-runtime = dotnet-sdk;
project = "Microsoft.CodeAnalysis.LanguageServer";
in
@@ -45,6 +45,14 @@ buildDotnetModule rec {
# see this comment: https://github.com/NixOS/nixpkgs/pull/318497#issuecomment-2256096471
# we can remove below line after https://github.com/dotnet/roslyn/issues/73439 is fixed
"-p:UsingToolMicrosoftNetCompilers=false"
+ # force everything to net9.0
+ "-p:NetRoslyn=net9.0"
+ "-p:NetRoslynSourceBuild=net9.0"
+ "-p:NetRoslynAll=net9.0"
+ "-p:NetRoslynBuildHostNetCoreVersion=net9.0"
+ "-p:NetRoslynNext=net9.0"
+ "-p:NetVSCode=net9.0"
+ "-p:NetVSShared=net9.0"
];
# two problems solved here:
@konradmalik what would you think of that? it allows all the reference dependencies to be removed.
There was a problem hiding this comment.
Another option is to look into forcing KnownAppHost to use the versions we actually have in nixpkgs.
There was a problem hiding this comment.
I'm fine with that if it works. Do you want to include it here, or should I create a PR? could try KnownAppHost there as well.
There was a problem hiding this comment.
We could also just leave it as @Smaug123 suggested. Not sure what it means for the deprecated dotnet 7 though.
Also an idea - we could try setting props such that this branch will be used
which, if I understand correctly, will force using only NetCurrent and NetPrevious which is set to net9.0 in arcade and net8.0 respectively.
All that being said - maybe I should do it in a separate PR aimed just at roslyn-ls.
There was a problem hiding this comment.
Also an idea - we could try setting props such that this branch will be used
I did try this, but DotNetBuildFromSource seems to cause other problems. I think it might be meant for building as part of the VMR?
I think I'll leave it as is for now. If we ever improve KnownAppHost in the build infrastructure, the dependencies will be reduced on the next fetch-deps, but only if we used the combined sdks.
|
Result of 2 packages marked as broken and skipped:
6 packages failed to build:
87 packages built:
|
|
All package tests on linux also passed, so this is ready for review. |
Smaug123
approved these changes
Aug 22, 2024
There was a problem hiding this comment.
Personally I'd stick with the "all the runtimes" in roslyn-ls if it has hard-coded requirements for it, rather than adding our own msbuild property overrides. But 1) I don't use roslyn-ls, 2) my opinion is weakly held, and 3) I value consistency with upstream quite highly to reduce work.
| @@ -34,6 +34,7 @@ in writeShellScriptBin "patch-nupkgs" ('' | |||
| if [ "$magic" = $'\177ELF' ]; then return 0; else return 1; fi | |||
| } | |||
| cd "$1" | |||
| '' + lib.optionalString stdenv.isLinux '' | |||
There was a problem hiding this comment.
This change looks obviously correct; do you know why this wasn't there before?
There was a problem hiding this comment.
For VMR, packages weren't being patched on darwin before, but 9.0 now needs the ilcompiler fix below.
I suppose this could actually use stdenv.hostPlatform.isElf.
There was a problem hiding this comment.
I think I'll leave this as-is for now. isElf is something to consider for the future, but everything else is using isLinux and isDarwin. I think realistically this can happen if/when we make dotnet work on BSD.
konradmalik
approved these changes
Aug 23, 2024
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
FYI @NixOS/dotnet
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.