-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
buildMavenPackage: Add overrideMavenAttrs
function and buildOffline
documentation
#313152
base: master
Are you sure you want to change the base?
Conversation
@ofborg build lemminx dbeaver java-language-server h2 scenebuilder schemaspy |
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.
Discussed at #ZurichZHF
}; | ||
passthru = | ||
let | ||
makeOverridableMavenPackage = f: origArgs: |
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.
makeOverridableMavenPackage = f: origArgs: | |
makeOverridableMavenPackage = build-package: args: |
This is the inner function defined in build-package.nix, right? We may want to be clear about that, and if you want to be even clearer, rename the file and the function to mavenRecipe
(because recipes is what we call functions that produce derivations). Using args
also matches the @args
in build-package (which we may want to move to the front for clarity).
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.
@roberth argues we may want a more specific names for the argument, such as mavenArgs
.
let | ||
makeOverridableMavenPackage = f: origArgs: | ||
let | ||
ff = f origArgs; |
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.
ff = f origArgs; | |
drv = build-package args; |
The result of this is a derivation, right?
makeOverridableMavenPackage = f: origArgs: | ||
let | ||
ff = f origArgs; | ||
overrideWith = newArgs: origArgs // (if lib.isFunction newArgs then newArgs origArgs else newArgs); |
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.
overrideWith = newArgs: origArgs // (if lib.isFunction newArgs then newArgs origArgs else newArgs); | |
overrideWith = newArgs: origArgs // lib.toFunction newArgs origArgs; |
@roberth says this is slightly less efficient because it always creates a function, but I agree it's (slightly) more readable
if builtins.isAttrs ff then | ||
(ff // { | ||
overrideMavenAttrs = newArgs: makeOverridableMavenPackage f (overrideWith newArgs); | ||
}) | ||
else if builtins.isFunction ff then { | ||
overrideMavenAttrs = newArgs: makeOverridableMavenPackage f (overrideWith newArgs); | ||
__functor = self: ff; | ||
} | ||
else ff; |
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.
if builtins.isAttrs ff then | |
(ff // { | |
overrideMavenAttrs = newArgs: makeOverridableMavenPackage f (overrideWith newArgs); | |
}) | |
else if builtins.isFunction ff then { | |
overrideMavenAttrs = newArgs: makeOverridableMavenPackage f (overrideWith newArgs); | |
__functor = self: ff; | |
} | |
else ff; | |
(ff // { | |
overrideMavenAttrs = newArgs: makeOverridableMavenPackage f (overrideWith newArgs); | |
}); |
This can never be anything but a derivation, see build-package.nix, which returns the output of mkDerivation
. Likely the conditional is a cargo-cult from previous ways of doing things.
@roberth Any idea on how to make this work with finalAttrs? jd-cli.overrideMavenAttrs (finalAttrs: old: rec {
... |
if builtins.isAttrs drv then | ||
(drv // { | ||
overrideMavenAttrs = newArgs: makeOverridableMavenPackage mavenRecipe (overrideWith newArgs); | ||
}) | ||
else if builtins.isFunction drv then { | ||
overrideMavenAttrs = newArgs: makeOverridableMavenPackage mavenRecipe (overrideWith newArgs); | ||
__functor = self: drv; | ||
} | ||
else drv; |
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.
if builtins.isAttrs drv then | |
(drv // { | |
overrideMavenAttrs = newArgs: makeOverridableMavenPackage mavenRecipe (overrideWith newArgs); | |
}) | |
else if builtins.isFunction drv then { | |
overrideMavenAttrs = newArgs: makeOverridableMavenPackage mavenRecipe (overrideWith newArgs); | |
__functor = self: drv; | |
} | |
else drv; | |
drv // { | |
overrideMavenAttrs = newArgs: makeOverridableMavenPackage mavenRecipe (overrideWith newArgs); | |
}; |
IIUC, the recipe function always returns attrs.
Code like this is confusing enough even without dead code.
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.
Yes, we actually already discussed this during review when we met.
I'm not 100% sure what it should mean, and while it could be done, the "architecture" - functions composed in sequence, but made overridable - does not make this easy, and requires that the logic to make that work have knowledge about all other relevant overriding functions, such as I'm sure you could make it work for limited use cases, e.g. initially not supporting |
4e613d3
to
e2d1c01
Compare
Description of changes
This pr does the following:
overrideMavenAttrs
function to make it possible to override attributes of thebuildMavenPackage
function.overrideMavenAttrs
function.buildOffline = true
argument for thebuildMavenPackage
function.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.