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

Jump to bottom

srcOnly: fix invocation and document #85750

Merged
merged 2 commits into from Apr 27, 2020
Merged

srcOnly: fix invocation and document #85750

merged 2 commits into from Apr 27, 2020

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Apr 22, 2020 •
edited

Motivation for this change

Previously, callPackage would try and fill the arguments such as name
and src which would cause problems if those existed as top-level
attributes. This also makes it clearer what part is the function
signature.

Then document the derivation inline in the code to explain the ellipsis
and various use-cases.

This is a follow-up on #80903

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.

@zimbatm zimbatm requested a review from edef1c April 22, 2020 07:50
@zimbatm
Copy link
Member Author

zimbatm commented Apr 22, 2020

The buildInputs argument really should be nativeBuildInputs if the goal is to provide some extra unpackers.

@zimbatm
srcOnly: fix invocation and document
bbbc419 
Previously, callPackage would try and fill the arguments such as `name`
and `src` which would cause problems if those existed as top-level
attributes. This also makes it clearer what part is the function
signature.

Then document the derivation inline in the code to explain the ellipsis
and various use-cases.
edef1c
edef1c approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@edef1c edef1c left a comment

Choose a reason for hiding this comment

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

LGTM.

The buildInputs argument really should be nativeBuildInputs if the goal is to provide some extra unpackers.

This seems reasonable, and probably deserves a followup commit. I suspect it'll break some usage, but the ripple effect there causes nixpkgs improvement if anything.

@zimbatm
Copy link
Member Author

zimbatm commented Apr 23, 2020 •
edited

@edef1c: pushed the nativeBuildInputs change.

Inside of nixpkgs, srcOnly is used in one place and doesn't use the buildInputs (or the ellipsis)

cdepillabout
cdepillabout approved these changes Apr 27, 2020
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Thanks for adding documentation!

@zimbatm
srcOnly: rename buildInputs to nativeBuildInputs
31ead7d 
The only reason to pass build inputs is to extend the unpackPhase with
custom unpack commands. Eg: add "unrar" to unpack rar sources. And those
should really be passed as native build inputs. Why? Because
nativeBuildInputs is for dependencies that are used at build time but
will not propagate as runtime dependencies. And also, cross-compilation.
@zimbatm
Copy link
Member Author

zimbatm commented Apr 27, 2020

fixed the evaluation and made the last commit backward-compatible

@zimbatm zimbatm merged commit 1bf7f5f into NixOS:master Apr 27, 2020
@zimbatm zimbatm deleted the srcOnly-docs branch April 27, 2020 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edef1c edef1c edef1c approved these changes

@cdepillabout cdepillabout cdepillabout approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zimbatm @edef1c @cdepillabout