Skip to content
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

add .overrideDerivation and .overrideAttrs to packages created with callPackages #18455

Merged
merged 1 commit into from
Feb 14, 2017

Conversation

danbst
Copy link
Contributor

@danbst danbst commented Sep 9, 2016

Motivation for this change

After I've change tomcat to callPackages in #18419 , I've noticed that pkgs.tomcat8.overrideDerivation attribute is lost. This PR is my (and @aneeshusa) vision on how to restore it.

Things done

[x] tested override and overrideDerivation for tomcat
[ ] tested everything else

@mention-bot
Copy link

@danbst, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @urkud and @oxij to be potential reviewers

mkAttrOverridable = name: pkg: pkg // {
override = newArgs: mkAttrOverridable name (f (finalArgs // newArgs)).${name};
};
mkAttrOverridable = name: pkg: makeOverridableGeneric (x: x."${name}") f finalArgs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like postProcess is always applied directly to the output of f in makeOverrideableGeneric, so why not just pass the composition of f and x: x."${name}" to makeOverrideable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, how had I missed that?..

mkAttrOverridable = name: pkg: pkg // {
override = newArgs: mkAttrOverridable name (f (finalArgs // newArgs)).${name};
};
mkAttrOverridable = name: pkg: makeOverridable (newArgs: (f newArgs).${name}) finalArgs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also rename finalArgs to origArgs? I think that is less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. There are args, auto, autoArgs, finalArgs and newArgs in one function! So in any case it is better to follow binding declarations than relying on mnemonics.

@danbst
Copy link
Contributor Author

danbst commented Sep 21, 2016

ping @edolstra

@domenkozar
Copy link
Member

domenkozar commented Jan 3, 2017

Instead we should rather add overrideAttrs since that's preferred now (and lazy).

@aneeshusa
Copy link
Contributor

@domenkozar, this doesn't add overrideDerivation directly but goes though makeOverridable, which adds override, overrideDerivation, and overrideAttrs, so that should already work. (overrideAttrs currently doesn't show up on the GitHub diff because this PR is old but is present if you look on the master branch.)

@danbst danbst force-pushed the override branch 2 times, most recently from 2a1af91 to bc532ff Compare February 12, 2017 13:59
…callPackages`/`callPackagesWith`

nix/nixUnstable, tomcatN and postgresqlNN use `callPackages` pattern, they have .override
attribute, but lack .overrideDerivation and recent .overrideAttrs.
Packages created with `callPackage` have all of those. Because .overrideDerivation function
is used in public, without this we can break code when refactoring callPackage -> callPackages.
@danbst danbst changed the title add .overrideDerivation to packages created with callPackages add .overrideDerivation and .overrideAttrs to packages created with callPackages Feb 12, 2017
@danbst
Copy link
Contributor Author

danbst commented Feb 12, 2017

Rebased to master, added motivation for a change to commit message, changed topic name, tested using .overrideAttrs. All is good

@ttuegel ttuegel merged commit a5b8d46 into NixOS:master Feb 14, 2017
@danbst danbst deleted the override branch January 2, 2019 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants