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

setup-hooks/multiple-outputs.sh: fallback to REMOVE if out isn't in outputs #109599

Closed
wants to merge 1 commit into from

Conversation

holymonson
Copy link
Contributor

@holymonson holymonson commented Jan 17, 2021

Motivation for this change

Typically out is in outputs, but for split packages it's not. In that case maintainer should handle outputs themself, so fallback removing default outputs.

This could also avoid the touch $out trick.

Fix #16182

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

…utputs

Typically out is in outputs, but for split packages it's not. In that case
maintainer should handle outputs themself, so fallback removing default outputs.

Fix NixOS#16182
@holymonson
Copy link
Contributor Author

cc @Profpatsch @vcunat

@vcunat
Copy link
Member

vcunat commented Jan 17, 2021

I'm not sure what you mean by "split packages". I guess some practical examples would help.

I think it's been very rare in nixpkgs not to have $out. One reason is that it's where typical $PREFIX goes (many tools have some notion like that).

I'm not sure about your use cases, but why REMOVE as the added fallback? Off the top of my head, I'd expect the first output to be a better choice, which could be done (I think) simply by $outputs instead of REMOVE. Or on the whole... perhaps it would be best to start by choosing some of the outputs as default and assign it to $out? Some hooks just need some kind of overall default output (say $PREFIX).

@holymonson
Copy link
Contributor Author

holymonson commented Jan 17, 2021

I'm not sure what you mean by "split packages". I guess some practical examples would help.

"split packages" is a derivation contains many commands, such as those in unixtools.
I'm rewriting adv_cmds and it provides many commands including ps, which is required by cmake in bootstrap stage, so it should be better split without building other unnecessary. (That's what we are doing in adv_cmds/boot.nix now.)

I think it's been very rare in nixpkgs not to have $out. One reason is that it's where typical $PREFIX goes (many tools have some notion like that).

I'm not sure about your use cases, but why REMOVE as the added fallback? Off the top of my head, I'd expect the first output to be a better choice, which could be done (I think) simply by $outputs instead of REMOVE. Or on the whole... perhaps it would be best to start by choosing some of the outputs as default and assign it to $out? Some hooks just need some kind of overall default output (say $PREFIX).

They're rare and mostly standalone commands without libs, so typical $PREFIX and the multiple-outputs.sh fixup are in fact meaningless in such cases. There is a trick to solve it by keeping out in outputs and touch $out at the end (dozens of search results in nixpkgs). I'm thinking why not getting rid of that trick since $out should not be necessary (make #16182 possible).

The REMOVE-fallback solution isn't nice I admit, so it's made drafted. I hope we could find a better way to disable all those directory mangling hooks.

@Profpatsch
Copy link
Member

I’d like this change, since it removes a slight annoyance, but I can’t really help with implementing it.

@Profpatsch
Copy link
Member

Problem is that it might break some things, and it’s hard to predict what will break if we change this now.

@Ericson2314
Copy link
Member

Does REMOVE mean silently remove? That sounds bad. I rather have it error.

@vcunat
Copy link
Member

vcunat commented Jan 17, 2021

Nothing in the current or planned PR should change cases where $out is used (the majority).

AFAIK REMOVE only works for the moveToOutput function – but yes, it's meant to change that move into a remove. The usual ${!outputFoo} will fail (I think we have that set for undefined variables in stdenv now), so standard configure phases would fail with it, I expect.

@holymonson
Copy link
Contributor Author

Close this since I'm not plan to go further. Hope some one could fix it in the future.

@holymonson holymonson closed this Jan 23, 2021
Staging automation moved this from WIP to Done Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants