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

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

Closed

Conversation

danieldk
Copy link
Contributor

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.

@collares
Copy link
Member

collares 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.

@worldofpeace
Copy link
Contributor

@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

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

@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

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
The new cargoSetupHook makes it easier to work with mixed projects. In
this case meson & ninja + rust.

See NixOS#114669 (comment)
@SuperSandro2000
Copy link
Member

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.

@jonringer
Copy link
Contributor

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants