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.
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: add override mechanism for nuget packages #339953
dotnet: add override mechanism for nuget packages #339953
Changes from all commits
13bd100
9a14832
e530139
e496425
04686b6
e788823
2d43ecc
d477621
7ea78aa
32ccfdc
1f6cd35
9a0be2f
5b31367
e5cb52b
2563bd7
6c5a9e5
cc9c59c
e6c700e
14c908c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
❤️
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 a breaking change unfortunately.
roslyn-ls
for example, was using$projectFile
, and needed to be changed to$dotnetProjectFiles
.It's a little unintuitive that something like:
doesn't work. @tie you did #339335. What do you think?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withstdenv[NoCC]
as withbuildDotnetModule
.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.
Makes sense. 👍
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.
That sounds great!
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.
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.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.