Skip to content

buildRustPackage: factor out setting up .cargo/config to cargoSetupHook#112500

Merged
Mic92 merged 4 commits intoNixOS:stagingfrom
danieldk:cargo-setup-hook
Feb 11, 2021
Merged

buildRustPackage: factor out setting up .cargo/config to cargoSetupHook#112500
Mic92 merged 4 commits intoNixOS:stagingfrom
danieldk:cargo-setup-hook

Conversation

@danieldk
Copy link
Contributor

@danieldk danieldk commented Feb 9, 2021 •
edited by FRidh
Loading

Motivation for this change

Factor out setting up .cargo/config from buildRustPackage to a new cargoSetupHook. This makes it possible to reuse this functionality as a hook in derivations that do not use buildRustPackage.

As an example, the second commit uses cargoSetupHook in the tokenizers Python package.

Since I am not very familiar with hooks, I have first attempted to make a hook for setting up .cargo/config. I can later also try to make other hooks, such as cargoBuild.

CC @FRidh @Mic92

Built some Rust packages to check if there are any regressions.

Related issues:

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 changed the title Cargo setup hook buildRustPackage: factor out setting up .cargo/config to cargoSetupHook Feb 9, 2021
@danieldk danieldk mentioned this pull request Feb 9, 2021
Closed
@FRidh FRidh added this to the 21.05 milestone Feb 9, 2021
@ofborg ofborg bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. labels Feb 9, 2021
@SuperSandro2000 SuperSandro2000 mentioned this pull request Feb 9, 2021
Closed
10 tasks
@danieldk danieldk marked this pull request as ready for review February 9, 2021 10:55
FRidh
FRidh reviewed Feb 9, 2021
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

The hook is cargo/rust version dependent, not? Maybe then keep it as a part of rustPlatform so we don't mix rust/cargo versions in a build.

SuperSandro2000
SuperSandro2000 reviewed Feb 9, 2021
View reviewed changes
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally 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: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Feb 9, 2021
SuperSandro2000
SuperSandro2000 reviewed Feb 9, 2021
View reviewed changes
@danieldk
Copy link
Contributor Author

danieldk commented Feb 9, 2021

The hook is cargo/rust version dependent, not? Maybe then keep it as a part of rustPlatform so we don't mix rust/cargo versions in a build.

It should be, the hook is defined in make-rust-platform.nix, and uses rust/cargo from the Rust platform. Or did you mean in the tokenizers derivation (let it take rustPlatform as an argument)?

@FRidh
Copy link
Member

FRidh commented Feb 9, 2021

It should be, the hook is defined in make-rust-platform.nix, and uses rust/cargo from the Rust platform. Or did you mean in the tokenizers derivation (let it take rustPlatform as an argument)?

Both. Note I did not have a closer look at make-rust-platform.nix.

@danieldk danieldk force-pushed the cargo-setup-hook branch 2 times, most recently from efcfa55 to 84499a7 Compare February 9, 2021 11:12
@danieldk
Copy link
Contributor Author

danieldk commented Feb 9, 2021

Both. Note I did not have a closer look at make-rust-platform.nix.

Changed the tokenizers derivation to user rustPlatform, should make it easier to override the platform as well.

FRidh
FRidh reviewed Feb 9, 2021
View reviewed changes
pkgs/build-support/rust/hooks/cargo-setup-hook.sh Outdated
Copy link
Member

@FRidh FRidh Feb 9, 2021

Choose a reason for hiding this comment

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

With fetchCargoTarball we need to pass another attribute. Not important for this PR though.

Copy link
Contributor Author

@danieldk danieldk Feb 9, 2021

Choose a reason for hiding this comment

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

Hmpf, good point. We could change fetchCargoTarball to take cargoSha256/cargoHash instead, but that would make the argument non-canonical for a fetcher.

@FRidh
Copy link
Member

FRidh commented Feb 9, 2021

cc also @adisbladis @DavHau for feedback on how you intend to handle this in your Python tools.

Mic92
Mic92 reviewed Feb 9, 2021
View reviewed changes
@FRidh
Copy link
Member

FRidh commented Feb 9, 2021

needs to target staging though

@danieldk
Copy link
Contributor Author

danieldk commented Feb 9, 2021

/rebase-staging

@github-actions github-actions bot changed the base branch from master to staging February 9, 2021 18:28
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 9, 2021
jonringer
jonringer approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM

@jonringer
Copy link
Contributor

jonringer commented Feb 9, 2021

needs to be properly rebased onto staging though, some conflicts

Mic92
Mic92 approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Looks good to me too, but I have not tested it in depth.

danieldk and others added 4 commits February 10, 2021 07:01
@danieldk
buildRustPackage: factor out setting up .cargo/config to cargoSetupHook
d083f41 
This makes it possible to reuse this functionality as a hook in
derivations that do not use buildRustPackage.
@danieldk
python3Packages.tokenizers: switch to buildPythonPackage
4a4da22 
Use the new cargoSetupHook to set up Cargo vendoring, so that we do
not need buildRustPackage anymore.
@primeos @danieldk
python3Packages.cryptography: 3.3.2 -> 3.4.2
da73f94 
Backwards incompatible changes: Support for Python 2 has been removed.
Note: This isn't a problem for Nixpkgs because
pythonPackages.cryptography is frozen at version 3.3.2.

Other important packaging changes: "Cryptography now incorporates Rust
code. Users building cryptography themselves will need to have the Rust
toolchain installed. Users who use an officially produced wheel will not
need to make any changes. The minimum supported Rust version is 1.45.0."
@danieldk
doc: describe cargoSetupHook in the Rust section
198dd77
@danieldk
Copy link
Contributor Author

danieldk commented Feb 10, 2021

Fix merge conflict that occurred after merging staging-next.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 10, 2021
aaronjanse
aaronjanse approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@aaronjanse aaronjanse left a comment

Choose a reason for hiding this comment

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

Diff looks good. I'm excited!

@danieldk
Copy link
Contributor Author

danieldk commented Feb 11, 2021 •
edited
Loading

Is this ready to merge?

@Mic92
Copy link
Member

Mic92 commented Feb 11, 2021

Started to testing with nix-shell -p cntr python3.pkgs.cryptography...

@Mic92 Mic92 merged commit 674bfaf into NixOS:staging Feb 11, 2021
@danieldk danieldk deleted the cargo-setup-hook branch February 11, 2021 10:42
@FRidh
Copy link
Member

FRidh commented Feb 11, 2021

Follow-up for maturin in #112804.

@danieldk danieldk mentioned this pull request Feb 15, 2021
Merged
10 tasks
This was referenced May 30, 2021
Merged
Open
@jian-lin jian-lin mentioned this pull request Aug 17, 2024
Draft
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@adisbladis adisbladis adisbladis approved these changes

@Mic92 Mic92 Mic92 approved these changes

@bhipple bhipple Awaiting requested review from bhipple

@prusnak prusnak Awaiting requested review from prusnak

@andir andir Awaiting requested review from andir

@LnL7 LnL7 Awaiting requested review from LnL7

@zowoq zowoq Awaiting requested review from zowoq

@primeos primeos Awaiting requested review from primeos

+3 more reviewers

@FRidh FRidh FRidh approved these changes

@jonringer jonringer jonringer approved these changes

@aaronjanse aaronjanse aaronjanse approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 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. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

21.05

Development

Successfully merging this pull request may close these issues.

8 participants

@danieldk @FRidh @Mic92 @jonringer @adisbladis @SuperSandro2000 @aaronjanse @primeos