dotnet: add override mechanism for nuget packages #339953
Conversation
Some packages assume TMPDIR is unshared, even in nix-shell.
This fixes nuget-to-nix in projects that use the source-built sdk and `linkNugetPackages`.
Unpacking to the build root was a bad idea. stdenv uses dumpVars() to create a file env-vars containing the entire environment. This was being installed in the derivation output, and since it contains lots of store paths, it was bloating the closure for every nuget package.
corngood
force-pushed
the
nuget-overrides
branch
from
0fc2fb2 to
e4ab08b
Compare
- stop binding attributes we don't care about (e.g. name, doCheck) - remove attributes we handle in nix (e.g. useAppHost) - inherit attributes with default values (e.g. packNupkg)
ofborg
bot
added
8.has: clean-up
10.rebuild-darwin: 11-100
10.rebuild-linux: 101-500
labels
corngood
force-pushed
the
nuget-overrides
branch
from
e4ab08b to
e536f71
Compare
github-actions
bot
added
the
8.has: documentation
|
Result of 137 packages built:
|
|
Result of 8 packages marked as broken and skipped:
82 packages built:
|
| removeAttrs args [ | ||
| "nugetDeps" | ||
| "installPath" | ||
| "executables" | ||
| "projectFile" | ||
| "projectReferences" | ||
| "runtimeDeps" | ||
| "runtimeId" | ||
| "disabledTests" | ||
| "testProjectFile" | ||
| "buildType" | ||
| "selfContainedBuild" | ||
| "useDotnet" | ||
| "useAppHost" | ||
| ] |
There was a problem hiding this comment.
There was a problem hiding this comment.
Can I ask, what's the motivation for prefixing dotnet-specific derivation args in the first place?
I struggle to see how name conflicts would happen, for instance.
There was a problem hiding this comment.
For buildDotnetModules I agree. I probably wouldn't have prefixed them.
For hooks like dotnet-sdk-setup-hook I'm trying to be careful about collisions, because I want that stuff to work well in a mixed build environment. Ideally I'd like it to be as easy to build a dotnet project with stdenv[NoCC] as with buildDotnetModule.
There was a problem hiding this comment.
For hooks like
dotnet-sdk-setup-hookI'm trying to be careful about collisions, because I want that stuff to work well in a mixed build environment.
Makes sense. 👍
This is a breaking change unfortunately.
roslyn-lsfor example, was using$projectFile, and needed to be changed to$dotnetProjectFiles.
From my perspective, we have the verbose/prefixed names for anything handled by the dotnet hook, since that hook may be used with arbitrary stdenv derivations and therefore should avoid potential name collisions.
The arguments used by buildDotnetModule could therefore almost be thought of as aliases, since the verbosity is redundant in that context.
Looking at it from this perspective, I don't think it is such a bad thing for buildDotnetModule to pass all its arguments to stdenv.
On reflection I think this is the least surprising behaviour, even if it does feel a little messy internally; an end user (packager) would likely expect any argument they've passed to a builder function to be also made available in stdenv.
Ideally I'd like it to be as easy to build a dotnet project with
stdenv[NoCC]as withbuildDotnetModule.
That sounds great!
There was a problem hiding this comment.
On reflection I think this is the least surprising behaviour, even if it does feel a little messy internally; an end user (packager) would likely expect any argument they've passed to a builder function to be also made available in stdenv.
One problem with that is sometimes, like with nugetDeps, there are negative consequences to including it.
Stripping them definitely feels wrong though. Like either it should be two separate attrset arguments, or one as an attribute in the other.
MattSturgeon
added
the
12.approvals: 1
corngood
force-pushed
the
nuget-overrides
branch
from
e536f71 to
37c847e
Compare
…ACKAGES Restore operations get extremely slow when there are a lot of paths in NUGET_FALLBACK_PACKAGES.
|
Getting this error trying to build: > patchPhase completed in 59 seconds
> Running phase: updateAutotoolsGnuConfigScriptsPhase
> Updating Autotools / GNU config script to a newer upstream version: ./src/runtime/src/native/external/zlib-ng/tools/config.sub
> Running phase: configurePhase
> ./.dotnet SDK directory exists...it will not be installed
> =========/private/tmp/nix-build-dotnet-vmr-9.0.0-preview.7.drv-0/dotnet-9.0.0-preview.7.24405.7/eng/detect-binaries.sh: line 135: 6460 Bus error: 10 /private/tmp/nix-build-dotnet-vmr-9.0.0-preview.7.drv-0/dotnet-9.0.0-preview.7.24405.7/.dotnet/dotnet run --project /private/tmp/nix-build-dotnet-vmr-9.0.0-preview.7.drv-0/dotnet-9.0.0-preview.7.24405.7/eng/tools/BinaryToolKit -c Release --property:RestoreSources=/nix/store/7zl5mn4dsynlbfws5ffi4zs4jlnkcnjw-Private.SourceBuilt.Artifacts.9.0.100-preview.7.24380.1.centos.9-x64 clean /private/tmp/nix-build-dotnet-vmr-9.0.0-preview.7.drv-0/dotnet-9.0.0-preview.7.24405.7 -o /private/tmp/nix-build-dotnet-vmr-9.0.0-preview.7.drv-0/dotnet-9.0.0-preview.7.24405.7/artifacts/log/binary-report -ab /private/tmp/nix-build-dotnet-vmr-9.0.0-preview.7.drv-0/dotnet-9.0.0-preview.7.24405.7/eng/allowed-sb-binaries.txt -l Debug -p CustomPackageVersionsProps=/nix/store/7zl5mn4dsynlbfws5ffi4zs4jlnkcnjw-Private.SourceBuilt.Artifacts.9.0.100-preview.7.24380.1.centos.9-x64/PackageVersions.propsiirc though, dotnet 9 is broken on darwin already. i'll try to just build one of the GUI applications. |
Hmm, I was able to build dotnet 9 on darwin, even in the sandbox. Maybe it's dependent on OS version? If you could try any of the avalonia apps, that would be great. |
corngood
force-pushed
the
nuget-overrides
branch
from
37c847e to
f6799a2
Compare
corngood
force-pushed
the
nuget-overrides
branch
from
f6799a2 to
14c908c
Compare
|
|
I'm running it again to see if there were any transient errors. It seems to take a while on a few packages. Dotnet 8 not building would trouble me more than 9.... Result of 8 packages marked as broken and skipped:
12 packages failed to build:
70 packages built:
|
|
Result of 8 packages marked as broken and skipped:
6 packages failed to build:
76 packages built:
|
wegank
removed
the
12.approvals: 1
|
@khaneliman interesting. I'm not sure why it worked for me. Both 8 and 9 are broken on trunk due to swift (#327836). In any case, I don't think there's a regression here. Thanks for testing the GUI app. That looks correct. |
Just a hunch, but IIRC nixpkgs-review merges the PR against the local checked-out nixpkgs branch (or maybe it fetches an up-to-date base-branch, I don't recall). So if you try building on this branch directly you may get a different result to if you try building with this branch merged onto a more up-to-date base-branch. |
For a PR, by default it fetches the target branch (master) and merges it. So we may not have been building the exact same thing. However, swift has been broken for quite a while on master, so I think there's some non-deterministic behaviour going on. It might be related to sandbox settings, OS versions, or just intermittent. |
Np :)
This is how I've treated darwin reviews for a bit... some stuff can be pretty consistent, but it doesn't surprise me when things aren't behaving the same for everyone with darwin in nixpkgs. Swift being a perfect example, I haven't had any issues building it locally, but it's been an issue for a lot of people and hydra.. |
| @@ -4,6 +4,8 @@ tmp=$(mktemp -d) | |||
| trap 'chmod -R +w "$tmp" && rm -fr "$tmp"' EXIT | |||
|
|
|||
| HOME=$tmp/.home | |||
| export TMPDIR="$tmp/.tmp" | |||
There was a problem hiding this comment.
It'd be nice to restore this after the script is over so people don't have to restart their shells
There was a problem hiding this comment.
It's not really meant to be sourced in a user shell. It's only meant to be called via nix-shell in the package-fetch-deps script.
I actually would have preferred to make it all work in one file with nix-shell shebangs, but I couldn't find a way to pass the derivation path, etc.
| mkNugetSource = callPackage ../../../build-support/dotnet/make-nuget-source { }; | ||
| mkNugetDeps = callPackage ../../../build-support/dotnet/make-nuget-deps { }; | ||
| fetchNupkg = callPackage ../../../build-support/dotnet/fetch-nupkg { }; |
There was a problem hiding this comment.
We can leave this for later, but it'd be nice to document these so people know what they are and what they do
There was a problem hiding this comment.
I don't think mkNugetSource/Deps serve a purpose now, and hopefully fetchNupkg can replace fetchNuGet.
| @@ -60,7 +64,7 @@ configureNuget() { | |||
| done | |||
| fi | |||
|
|
|||
| if [[ -f paket.dependencies ]]; then | |||
| if [[ -z ${keepNugetConfig-} && -f paket.dependencies ]]; then | |||
There was a problem hiding this comment.
Also needs to be documented (I know it already existed, but I forgot to mention this when it was added)
There was a problem hiding this comment.
It doesn't really fit in the buildDotnetModule docs, because you shouldn't have to worry about it there, but i do think there could be a whole section about the nuget package and dotnet-sdk hooks, and the variables they use: linkNugetPackages, etc.
|
Anyone have any thoughts on casing of From VMR :) A variable starting with Maybe we try to stick with I've added some |
|
I think ideally we should try to follow the project's naming, so your idea of using either Typing it is more annoying but I don't know if that warrants not leaving the G uppercase. |
|
Caused minor eval failures. Proposed the change as: |
Description of changes
Cc: @NixOS/dotnet
This is some of the stuff from #336824, plus a bunch of fixes for problems I found along the way. I've built all packages before and after a
fetch-deps. I've run all the packages that depend on avalonia to ensure they at least show a UI.various fixes to
fetch-deps, mostly knock-ons of the switch to unpacked packagesclean up
buildDotnetModuleargument attribute setsStripping out what's used in nix, inheriting default values, etc.
mkNugetDeps/SourceintodotnetCorePackages(still inherited in all-packages)dotnetCorePackages.fetchNupkg** frommkNugetDeps, also indotnetCorePackagesI wanted the fetcher to be exposed, but separated from the obsolete (?)
fetchNuGetto make it a little less confusing.mkNugetDepsfrombuildDotnetModulefetchNupkgThis is the important architectural change. Adding all the nuget deps as build inputs allows them to specify hooks, propagate dependencies, etc.
The override mechanism allows patching nuget packages by name. As an example I've dealt with some of the dependencies of skiasharp and avalonia.x11, and pulled the runtime deps out of the dependent packages.
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.