buildDotnetModule: add finalAttrs support#331398
Conversation
|
I'm happy to take this over, but I'm honestly not sure what I'd do differently. As for testing, we could have a test similar to |
ofborg
bot
added
10.rebuild-darwin: 0
MattSturgeon
force-pushed
the
dotnet-finalAttrs
branch
from
b146125 to
cc9d1b2
Compare
ofborg
bot
added
8.has: package (new)
|
This all looks good to me. Anything I can do to help? |
I think it's just the test that is incomplete. I've not had chance to look at it since I pushed it up, but I wanted to come up with some test cases that'd check Any ideas/suggestions/etc there would be helpful! I also wonder if the implementation can be simplified using |
|
Please ignore nixf-tidy in both cases. See #331085 |
MattSturgeon
force-pushed
the
dotnet-finalAttrs
branch
from
31e2766 to
51035fe
Compare
MattSturgeon
changed the title
finalAttrs supportfinalAttrs support
MattSturgeon
force-pushed
the
dotnet-finalAttrs
branch
from
51035fe to
f3ba1f6
Compare
Allow users to pass arguments to `buildDotnetModule` in the form:
```nix
buildDotnetModule (finalAttrs: {
# Args
})
```
Exposing the behaviour of the underlying `mkDerivation` and allowing
packages to be defined in a recursive way that works correctly even when
the package is overridden, e.g. using `overrideAttrs`.
Added some simple test cases that piggyback on the existing
`structured-attrs` test.
MattSturgeon
force-pushed
the
dotnet-finalAttrs
branch
from
f3ba1f6 to
79d2604
Compare
|
@corngood I've polished up the test a little and included a test for overriding the copyright string. I think this is ready for review. Thanks for your support so far! I've checked the tests pass; both |
corngood
left a comment
corngood
left a comment
There was a problem hiding this comment.
Looks good. FYI @NixOS/dotnet in case anyone else can review.
| stdenvNoCC.mkDerivation ( | ||
| finalAttrs: | ||
| let | ||
| args = if lib.isFunction fnOrAttrs then fnOrAttrs (args // finalAttrs) else fnOrAttrs; |
There was a problem hiding this comment.
Only just got word of the fact that buildDotnetModule has finalAttrs support at wanted to share my thoughts.
(I know this line has been updated since, but it's a bit more obvious what's happening here)
Keep in mind that in the current version, the non-prefixed names (e.g executables) are filtered out of the args after transformation.
AFAICT args // is there so that you can access the non-prefixed values that have been passed to the builder.
This means since they are not passed to the builder, they are not actually part of finalAttrs so you cannot override them.
However you can override the prefixed names.
BUT if you used the non-prefixed name instead of the prefixed name when getting the value from ""finalAttrs"" (aka. args // finalAttrs), you will not get the overridden value.
Do we really need args //? Couldn't the users just do lib.fix or rec if they wanted to access those values? I know it would incur extra noise, but at least they wont have false expectations.
There was a problem hiding this comment.
For added context, at the time of this PR args was much closer to the finalAttrs, with only nugetDeps being removed. Now there's a few more differences:
nixpkgs/pkgs/build-support/dotnet/build-dotnet-module/default.nix
Lines 214 to 229 in 5a58822
As per #331398 (comment), the "correct" approach would probably be to add a overrideDotnetAttrs function, similar to python's overridePythonAttrs.
5 participants
Description of changes
I've updated the description now that this is ready for review. See the original description below:
Draft PR description
This is an untested proof of concept for adding
finalAttrssupport tobuildDotnetModule, as discussed #331355 (comment)I've opened this as a draft just to share my conceptual work, it'd probably be best if someone more familiar with build-support/dotnet would take over. @corngood is this something you'd be willing to do?
Otherwise I can continue to work on this and improve based on feedback.
Presumably, we'd also want tests somewhere for this? I'm unfamiliar with how build-support functions are usually tested.
It might be nice to reformat using rfc166 here too, since almost the entire file is being touched anyway...
Allow users to pass arguments to
buildDotnetModulein the form:Exposing the behaviour of the underlying
mkDerivationand allowing packages to be defined in a recursive way that works correctly even when the package is overridden, e.g. usingoverrideAttrs.Added some simple test cases that piggyback on the existing
structured-attrstest.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.