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.

Sign up for GitHub

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

buildRustPackage: support multiple cargo invocations #105825

Closed
wants to merge 1 commit into from
Closed

buildRustPackage: support multiple cargo invocations #105825

wants to merge 1 commit into from

Conversation

steveej
Copy link
Contributor

@steveej steveej commented Dec 3, 2020

Motivation for this change

This change allows calling cargo build multiple times with distinct
flags. Please see the doc changes for more details.

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.

@symphorien
Copy link
Member

what is the use case ?

@steveej
Copy link
Contributor Author

steveej commented Dec 4, 2020 •
edited

@symphorien

what is the use case ?

I just pushed a new commit to demonstrate using different features for different crates within the same workspace. Does that help understanding the use-case?

@symphorien
Copy link
Member

I see. Just so we can test, can you name a crate/project where this is needed?

@steveej
Copy link
Contributor Author

steveej commented Dec 6, 2020

@symphorien of course. I'd like to integrate the changes made here with holochain/holochain#438. The binaries there share a large portion of common dependencies so it is rather inefficient to use separate packages.

@lopsided98
Copy link
Contributor

@steveej Have you seen crate2nix? It might be able to solve your problem in a more elegant way.

@steveej
Copy link
Contributor Author

steveej commented Dec 15, 2020

@lopsided98

Have you seen crate2nix? It might be able to solve your problem in a more elegant way.

I hadn't seen that, thanks. I would prefer if the mechanism lived in nixpkgs itself, so I'm suggesting that we do merge this PR. I'll also take a look at crate2nix nonetheless.

@steveej
Copy link
Contributor Author

steveej commented Dec 19, 2020

@lopsided98 @symphorien is there anything blocking this that I can help with?

@symphorien
Copy link
Member

I must acknowledge that I don't really like this API. Possible suggestions:

  • define a bash level function runCargo which runs cargo with all the default flags, and then it's possible to do
{
preBuild = ''
   runCargo --manifest-path foo bar
'';
}
  • If the only possible use case is with different manifest paths, then make it possible to set cargoBuildFlags to a set with the path to the manifest as key and the flags as values.

What do you think?

@steveej
Copy link
Contributor Author

steveej commented Jan 5, 2021

@symphorien

I must acknowledge that I don't really like this API. Possible suggestions:
(...)
What do you think?

Well, I think we're unfortunately entering bike-shedding territory here. I prefer a Nix level solution or else I wouldn't have written it this way 😉 However, if you commit to approving the solution you outlined I'll implement it so we can get the functionality merged.

@symphorien
Copy link
Member

Let's wait for another opinion then.

@otavio
Copy link
Contributor

otavio commented Feb 21, 2021

I think this is quite nice and indeed a nice addition to the system. I think using the nix system seems more ergonomic and natural to me. I support the @steveej proposal.

@steveej steveej force-pushed the pr/buildrustpackage-workspace-features branch 2 times, most recently from 20188c6 to 2c42bfd Compare February 23, 2021 09:40
@steveej
Copy link
Contributor Author

steveej commented Feb 23, 2021

Thanks for the review @otavio. I rebased the PR and think we can merge this now. I haven't squashed the commits as I was previously told it makes reviewing harder.

To the one who will merge this: please use the squash & merge button 🙂

@SuperSandro2000 SuperSandro2000 requested review from danieldk and removed request for andir February 23, 2021 10:24
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Feb 23, 2021 •
edited

To the one who will merge this: please use the squash & merge button slightly_smiling_face

Just squash them together as the chances that someone forgets it are pretty high. Also this needs to go through staging.

@steveej
buildRustPackage: support multiple cargo invocations
8d87be1 
This change allows calling `cargo build` multiple times with distinct
flags. Please see the doc changes for more details.
@steveej steveej force-pushed the pr/buildrustpackage-workspace-features branch from f7f3875 to 8d87be1 Compare February 23, 2021 10:52
@steveej
Copy link
Contributor Author

steveej commented Feb 23, 2021

Just squash them together as the chances that someone forgets it are pretty high.

Done 👍

Also this needs to go through staging.

You mean I should target the staging branch rather than master?

@danieldk
Copy link
Contributor

danieldk commented Feb 23, 2021 •
edited

You mean I should target the staging branch rather than master?

Yes, but that also requires quite some rewriting of this PR. buildRustPackage was completely rewritten to be hook-based (in order to support use of Cargo in e.g. Python packages). These changes are now in staging-next (they were merged to staging, because they require mass rebuilds). But until staging-next is merged into master, we shouldn't make any buildRustPackage changes in master to avoid merge conflicts.

danieldk
danieldk suggested changes Feb 23, 2021
View reviewed changes
pkgs/build-support/rust/default.nix
@@ -35,6 +35,7 @@
, cargoUpdateHook ? ""
, cargoDepsHook ? ""
, cargoBuildFlags ? []
, cargoBuildFlagsMultiple ? []
Copy link
Contributor

@danieldk danieldk Feb 23, 2021 •
edited

Choose a reason for hiding this comment

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

I agree with @symphorien . And I don't think it is bikeshedding, API complexity and maintainability should be considered in changes. The buildRustPackage API is already quite complex as it is. Unless this use-case becomes very frequent in nixpkgs, I think it is better that projects that need to do multiple builds with different feature flags use their own buildPhase.

As @lopsided98 mentions, such cases are very well handled by buildRustCrate plus crate2nix, since it compiles every crate as a separate output path.

I hadn't seen that, thanks. I would prefer if the mechanism lived in nixpkgs itself, so I'm suggesting that we do merge this PR. I'll also take a look at crate2nix nonetheless.

crate2nix is in nixpkgs and uses the buildRustCrate functionality in nixpkgs. If you do not want to add a Cargo.nix file in your repository and keep it up to date, crate2nix also supports IFD to generate the Nix from Cargo.lock.

@steveej steveej mentioned this pull request Apr 19, 2021
Closed
@stale
Copy link

stale bot commented Aug 22, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 22, 2021
@steveej
Copy link
Contributor Author

steveej commented Aug 25, 2022

i haven't been using this myself so i won't pursue this PR any longer

@steveej steveej closed this Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk requested changes

@otavio otavio otavio approved these changes

@zowoq zowoq Awaiting requested review from zowoq zowoq is a code owner

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: rust 8.has: documentation 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@steveej @symphorien @lopsided98 @otavio @SuperSandro2000 @danieldk