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, fetchcargo: optionally use the real config from cargo vendor #45889

Closed
wants to merge 1 commit into from
Closed

buildRustPackage, fetchcargo: optionally use the real config from cargo vendor #45889

wants to merge 1 commit into from

Conversation

erictapen
Copy link
Member

Motivation for this change

Current fetchcargo behaviour is not sufficient, when sources different from crates.io are used. An example where this makes packaging impossible is #44465.

As a naive change of the code would result in breaking cargoSha256 all over the place, an additional argument is proposed to turn the new behaviour on.

Things done

This introduces useRealVendorConfig as a new argument to buildRustPackage. Whenever buildRustPackage fetches Cargo dependencies and useRealVendorConfig is true, fetchcargo writes its generated config into the vendor output.

Imho this behaviour should be desirable all the time. A possible migration path is proposed in the commit message.

This should trigger a rebuild in every package using cargoSha256.

How I tested this
  • nix-build -A rustracer --check.
  • Changed cargoSha256 of rustracer, rebuilt, still wants the old hash.
  • nix-build -A cargo --check, a package which uses buildRustPackage but not fetchcargo, same drv as on master.
  • Packaged sequoia-pgp with useRealVendorConfig = true, a package with dependencies from non-crates.io sources.

Pinging @symphorien @madjar @cstrahan @wizeman @globin @Havvy @wkennington

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@erictapen erictapen mentioned this pull request Aug 31, 2018
Closed
9 tasks
@symphorien
Copy link
Member

Great!
I double checked:

  • nox-review says no hash changed
  • nix-du without useRealVendorConfig still requires the same hash
  • nix-du builds with useRealVendoConfig
  • rls (which I intend to package and requires this fix) builds fine
  • The output of fetchcargo does not depend on the derivation name

The only thing which is missing is an update of the documentation
./doc/languages-frameworks/rust.section.md
to mention the useRealVendorConfig and use it in the example (so that newly written copy-pasted code uses it).

@symphorien
Copy link
Member

Maybe even release notes so that we can deprecate the old behavior on 19.03 ?

@erictapen
Copy link
Member Author

Glad you like it! Do you think this could make it into 18.09? I'll try to add documentation in the next few hours.

@erictapen
Copy link
Member Author

I'll try to add documentation in the next few hours.

Ok, can't work on it earlier than tomorrow.

@erictapen
Copy link
Member Author

Changed my commit to include documentation and release notes.

@erictapen
buildRustPackage, fetchcargo: optionally use vendor config from cargo…
c90de4c 
…-vendor

By setting useRealVendorConfig explicitly to true, the actual (slightly
modified) config generated by cargo-vendor is used.

This solves a problem, where the static vendor config in
pkgs/build-support/rust/default.nix would not sufficiently replace all
crates Cargo is looking for.

As useRealVendorConfig (and writeVendorConfig in fetchcargo) default to
false, there should be no breakage in existing cargoSha256 hashes.

Nethertheless, imho using this new feature should become standard. A
possible deprecation path could be:

- introduce this patch
- set useRealVendorConfig explicitly to false whereever cargoSha256 is
  set but migration is not wanted yet.
- after some time, let writeVendorConfig default to true
- when useRealVendorConfig is true everywhere cargoSha256 is set and
  enough time is passed, `assert cargoVendorDir == null ->
  useRealVendorConfig;`, remove old behaviour
- after some time, remove all appearences of useRealVendorConfig and the
  parameter itself
@erictapen
Copy link
Member Author

and rebased to current master...

Mic92
Mic92 reviewed Sep 3, 2018
View reviewed changes
pkgs/build-support/rust/default.nix
EOF
'' + (if useRealVendorConfig then ''
sed "s|directory = \".*\"|directory = \"$(pwd)/$cargoDepsCopy\"|g" \
"$(pwd)/$cargoDepsCopy/.cargo/config" > .cargo/config
Copy link
Member

Choose a reason for hiding this comment

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

Not that someone tried this before and there the idea was to enable it by default.
I have a better feeling if we only do this on a subset on packages like this pull request does, since the output of cargo-vendor can still change in future.

Can you also add a check to read the generated configuration of cargo-vendor and emits an error if the it contains git repositories but useRealVendorConfig is not enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact, that there are so many prior issues and PRs about this is annoying. I really should've done my research, sorry for that.

Also, the fact that the output of cargo-vendor can change really did not come to my mind. As you say it, it is clear to me that my PR wouldn't be the good solution I was hoping for, but more of a hack.

I wonder if it would be better to close this PR and open another one, as your proposal really changes the direction where this is going?

Copy link
Member

@Mic92 Mic92 Sep 5, 2018 •
edited

Choose a reason for hiding this comment

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

I think what could work instead is, having a script with a toml parser (e.x.: python), that reads the cargo config and produces cargo config for the fixed-output derivation. That way we can control the output and can enable this by default.

Mic92
Mic92 reviewed Sep 3, 2018
View reviewed changes
doc/languages-frameworks/rust.section.md
that currently this parameter defaults to `false` only due to compatibility
reasons, as setting this to `true` requires a change in `cargoSha256`.
Eventually this distinction will be deprecated, so please always set
`useRealVendorConfig` to `true` and make sure to recompute the `cargoSha256`.
Copy link
Member

@Mic92 Mic92 Sep 3, 2018 •
edited

Choose a reason for hiding this comment

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

For this to become the default I would like to see our own script generating .cargo/config to avoid breaking checksums. cargo-vendor's output is not supposed to be stable: https://github.com/NixOS/nixpkgs/pull/34034/files#r162713377
That's why I would not set it by default.

@erictapen
Copy link
Member Author

Unfortunately I have near to no time for this atm. Please go ahead, if anyone wants to finish this.

@Mic92
Copy link
Member

Mic92 commented Sep 5, 2018

To repeat my comment so it will not get lost:
The best long-term solution would be if we have a script with a toml parser (e.x.: python), that reads the generated cargo config and produces new cargo config for the fixed-output derivation. That way we can control the output and can ensure its stability across different versions of cargo-vendor.

@symphorien symphorien mentioned this pull request Sep 8, 2018
Merged
9 tasks
@symphorien
Copy link
Member

Unfortunately I have near to no time for this atm. Please go ahead, if anyone wants to finish this.

#46362 is a continuation of this.

@erictapen
Copy link
Member Author

Close, as #46362 was merged. Yay!

@erictapen erictapen closed this Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: rust 8.has: changelog 8.has: documentation 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
4 participants
@erictapen @symphorien @Mic92 @GrahamcOfBorg