Skip to content

Comments

rustPlatform.cargoBuildHook: do not use when buildPhase is set#114669

Closed
danieldk wants to merge 1 commit intoNixOS:stagingfrom
danieldk:cargo-build-hook-overridable
Closed

rustPlatform.cargoBuildHook: do not use when buildPhase is set#114669
danieldk wants to merge 1 commit intoNixOS:stagingfrom
danieldk:cargo-build-hook-overridable

Conversation

@danieldk
Copy link
Contributor

@danieldk danieldk commented Feb 28, 2021

Motivation for this change

See #114659.

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.

@danieldk danieldk requested review from andir and zowoq as code owners February 28, 2021 15:09
@collares
Copy link
Member

collares commented Feb 28, 2021 •
edited
Loading

Would it be appropriate to land this on master instead of staging, since it fixes a channel blocker? There are 9 packages that use buildRustPackage and do buildPhase = null; in nixpkgs.

@ofborg ofborg bot added 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Feb 28, 2021
@worldofpeace
Copy link
Contributor

worldofpeace commented Feb 28, 2021

@collares I think this change make sense, but I think we're agreed on that packages like gnome-tour and fractal should add rustc and cargo to nativeBuildInputs, the meson files try to do this anyway, and just use cargoSetupHook. We were only hacking around to just use it for setup anyway (IIRC).

@worldofpeace
Copy link
Contributor

worldofpeace commented Feb 28, 2021

i.e I can just deliver those smaller fixes to master, and this can go through staging. But if @danieldk disagrees with what I just said than I would reconsider.

@collares
Copy link
Member

collares commented Feb 28, 2021 •
edited
Loading

@worldofpeace That's even better! I just suggested landing this on master because it would reduce the urgency of fixing it the "proper" way for all affected packages, but if you were already planning to fix the channel-blocking ones quickly then it's definitely the best approach. Thanks a lot!

Since I guess most packages won't be channel-blocking, here's a list of maintainers of affected packages (excluding gnome-tour and fractal which were already mentioned):

If you were tagged above, you were using buildRustPackage but overriding its buildPhase, which might be a sign that you can benefit from recent improvements in the Rust infrastructure (cargoSetupHook in particular).

@danieldk
Copy link
Contributor Author

danieldk commented Feb 28, 2021 •
edited
Loading

i.e I can just deliver those smaller fixes to master, and this can go through staging. But if @danieldk disagrees with what I just said than I would reconsider.

I fully agree. Merging this results in a lot of rebuilds.

kira-bruneau added a commit to kira-bruneau/nixpkgs that referenced this pull request Feb 28, 2021
@kira-bruneau
newsflash: use cargoSetupHook instead of buildRustPackage
6e3988a 
The new cargoSetupHook makes it easier to work with mixed projects. In
this case meson & ninja + rust.

See NixOS#114669 (comment)
@kira-bruneau kira-bruneau mentioned this pull request Feb 28, 2021
Merged
10 tasks
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Feb 28, 2021

Would it be appropriate to land this on master instead of staging, since it fixes a channel blocker? There are 9 packages that use buildRustPackage and do buildPhase = null; in nixpkgs.

No, because then everyone would need to rebuild all rust and python packages for the next half day or day.

@collares collares mentioned this pull request Mar 1, 2021
Merged
10 tasks
@jonringer
Copy link
Contributor

jonringer commented Mar 1, 2021

I think this is superceded by #114716

@danieldk
Copy link
Contributor Author

danieldk commented Mar 1, 2021

I think this is superceded by #114716

Yep, that's a superset. Closing this.

@danieldk danieldk closed this Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andir andir Awaiting requested review from andir

@zowoq zowoq Awaiting requested review from zowoq

Assignees

No one assigned

Labels

6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danieldk @collares @worldofpeace @SuperSandro2000 @jonringer