-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
dotnet: use unpacked packages in store #327651
Conversation
c9733ce
to
14186bc
Compare
Result of 4 packages failed to build:
141 packages built:
|
pkgs/build-support/dotnet/build-dotnet-module/hooks/dotnet-check-hook.sh
Show resolved
Hide resolved
Looks good to me so far.
Maybe if in The issue is that nuget lockfiles don't support multi-RID restores, as it only works for the current RID, and if you put in a dependency for a different RID in the lockfile it'll complain that it doesn't need it.
Maybe if we keep only the nuspec file in there we could somehow hack the restore algo into supporting it, but I don't know how I feel about it, feels a bit flaky.
I like this idea, I think it'd make things less confusing as one is supposed to be the target directory and the other a source. |
I'll do some more testing around this. What's in here right now might not be required for all packages. We could possibly allow multiple options (best first):
One problem with this is that it obfuscates runtime deps, unless we make the .nupkgs uncompressed, or extract the dependencies. |
I might be mistaken, but 1 and 2 seem to be the same in practice? They do seem like the best option considering that it's immutable and Given that most projects don't have lockfiles at all, I don't think we'll be able to get rid of overriding the source with a directory full of nupkgs as it'll try to use online sources instead if we don't do it. For now I think the most realistic path would be to proceed with 4 as it works on projects without lockfiles and maybe look into requiring lockfiles for projects or something similar so that we can get rid of the directory full of |
Yeah, more or less. It's just that
Currently
One thing I'm not sure of, and was going to test, is if we can do something like put empty files in place of the .nupkg files. If it's only using the source to determine which versions are available, maybe that'll work? Another crazier option might be to make a fake http source using a daemon? |
It actually supports multiple, but they need to be separated by semicolons:
How so? I thought it only searched through stuff that
I've asked a question in the .NET Discord to the nuget folks about it, will report back if I get anything, but I think we could get away with putting in a The fake HTTP daemon wouldn't work because it'd still try to download |
Ah yeah, I don't know why I thought that. I swear I looked it up in the code before, but it's definitely splitting on
It searches
I wasn't sure if it would actually download them or just enumerate the available versions.
This is a good idea to try. |
It does seem to work with an uncompressed zip containing only the nuspec. Something like that would be safe to put in outputs. |
14186bc
to
cc31676
Compare
Result of 12 packages failed to build:
135 packages built:
|
cc31676
to
02099a5
Compare
I've updated the description to match the latest version. Now we:
We also unconditionally:
In theory this would be optional if all dependencies were locked, but that's rare enough that I'm not sure it makes sense for it to be configurable yet. If
This is required to work around upstream bugs like tunnelvisionlabs/ReferenceAssemblyAnnotator#94 If
This is required for |
02099a5
to
53decad
Compare
Result of 6 packages failed to build:
141 packages built:
|
(I'm currently on holiday and diligently not doing anything that looks like work, but would in principle be interested in 1. this PR and 2. being on the dotnet team!) |
@Smaug123 have a good holiday and feel free to make a PR to add yourself here: nixpkgs/maintainers/team-list.nix Line 211 in a1159ff
|
By the way, don't take my "I'm interested" as a desire to block this PR on my account if it's ready to go in before I get back! |
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.
@inclyc
.
I'm a review bot that based on git-diff thus there might be comments to previous code. Our nixpkgs CI will skip them, but you can also fix them if you like!
e3b0fd5
to
f32ec36
Compare
Result of 147 packages built:
|
This is actually breaking some tests, like
Working on that now. |
This allows fetch-deps to work even when the deps file is missing.
f32ec36
to
2ae0a24
Compare
2ae0a24
to
f01dc2c
Compare
I fixed another problem with |
f01dc2c
to
d3ca502
Compare
I'm planning on merging this soon. I just wanted to check out some closure sizes. Some things will be larger uncompressed, but should be similar compressed. I also checked a few packages to confirm that build times are improved, and it seems likely to save in the neighborhood of 30s from restore in a package with a reasonable number of dependencies. |
Description of changes
This is still a bit of a WIP, although it does pass nixpkgs-review on linux and darwin, and fetch-deps works on everything in nixpkgs. I mostly just want to do some more cleanup passes and pull out some incidental changes into separate commits.
The idea here is that we keep packages in
$out/share/nuget/packages/[name]/[version]
, unpacked, in a structure that's compatible with what you'd have in~/.nuget/packages
. Then in a hook, we point$NUGET_FALLBACK_PACKAGES
at all the dependencies. We also create a source, which contains 'stub'.nupkg
files, containing only the.nuspec
, and add it to the rootnuget.config
.There are still some rough edges. For example,
dotnet tool restore
doesn't really play nice with$NUGET_PACKAGES
, and we still need to treat the package directory as a 'source' for projects that don't have locked dependencies. In order to deal with this, we passinstallable = true
tomkNugetDeps
, which tells it to create full.nupkg
files instead of stubs.Other highlights:
projectReferences
is now obsolete, and packages can be used from any build inputFYI @NixOS/dotnet @Smaug123 (aside: would you like to be added to the dotnet team?)
Questions:
.nupkg
generation for (dependent) build time, and keep them out of the store altogether?Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.