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

[20.03] buildRustCrate fixes backports #83601

Merged
merged 11 commits into from
Apr 3, 2020

Conversation

andir
Copy link
Member

@andir andir commented Mar 28, 2020

Motivation for this change

This PR backports a bunch of changes that have been done to buildRustCrate since the branch-off. They are (mostly) just fixing bugs. Some of which are more annoying then others.

Since none of these break compatibility with older packages or users I do think they are still good to be included.

The PRs that are included in this:

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

andir and others added 9 commits March 28, 2020 16:06
By overriding each dependency on every level of the dependency tree we
are creating a lot of unnecessary instances of the same derivation

Looking at the output size of `nix-instantiate --trace-function-calls
-vvvv …` and the execution time I got about a 10x improvement after
applying this change.

It was probably good intentions that lead to these overrides but in
practice no tooling (that I know of) really needs this. `carnix` and
`crate2nix` are fine without those overrides. Furthermore I believe that
it is the job of the tooling around `buildRustCrate` to provide a
coherent set of overrides. By not enforcing all of the overrides, debug
flags, verbosity, … to be the same throughout the closure we also allow
consumers to override specific aspects of the crates. Some (older?)
crates might need different `crateOverrides` then newer crates with the
same name. Currently such situations can not (easily) be implemented
with the override in-place.

(cherry picked from commit be5597f)
* Make errors include the crate name and make them much more prominent.
* Move more code into lib.sh
* Already source generated logging code and lib.sh in configure

(cherry picked from commit 04e7462)
…sub directories

This is what cargo does for git repositories.

See related issues:

* nix-community/crate2nix#53
* nix-community/crate2nix#33

(cherry picked from commit 8a6638d)
According to the Cargo documentation:

> The build script does not have access to the dependencies listed in
> the dependencies or dev-dependencies section (they’re not built
> yet!). Also, build dependencies are not available to the package
> itself unless also explicitly added in the [dependencies] table.

https://doc.rust-lang.org/cargo/reference/build-scripts.html

This change separates linkage of regular dependencies and build
dependencies.

(cherry picked from commit ea6e048)
This is a slight readability boost, I think.

(cherry picked from commit 7533876)
Linkage order is significant and sorting can result in link errors.

(cherry picked from commit d8b8537)
@ofborg ofborg bot added 6.topic: rust 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 28, 2020
andir and others added 2 commits March 29, 2020 13:03
As it turns out Darwin does most of the things differently then "normal"
systems. They are using a different shared library extension and require
an obscure commandline parameter that has to be added to every build
system out there. That issue seems to be with clang on Darwin as on
Linux that flag isn't required to build the very same tests (when using
clang).

After adjusting these two details the tests are running fine on the
darwin box that I was able to obtain.

(cherry picked from commit c8de31b)
...and remove superfluous dependency files (*.d).
...and copy dSYM directories on Mac OS when in release=false mode.

(cherry picked from commit 782b304)
@andir
Copy link
Member Author

andir commented Mar 29, 2020

@GrahamcOfBorg build buildRustCrateTests

@andir andir marked this pull request as ready for review March 31, 2020 09:43
@andir
Copy link
Member Author

andir commented Mar 31, 2020

Dear @NixOS/nixos-release-managers,

Please consider this for inclusion into 20.03. This carries a bunch of bug fixes and other QoL improvements. Since we have a very low amount of actual buildRustCrate users in nixpkgs the impact is very minimal. There are a few rebuilds but no new failures.

It would be great to have this in 20.03 since it also improves the compatibility with Darwin.

@flokli
Copy link
Contributor

flokli commented Mar 31, 2020

20.03 hasn't been released, should be fine IMHO to merge if it doesn't cause immediate breakages.

@andir andir added this to the 20.03 milestone Apr 3, 2020
@worldofpeace
Copy link
Contributor

Dear @NixOS/nixos-release-managers,

Please consider this for inclusion into 20.03. This carries a bunch of bug fixes and other QoL improvements. Since we have a very low amount of actual buildRustCrate users in nixpkgs the impact is very minimal. There are a few rebuilds but no new failures.

It would be great to have this in 20.03 since it also improves the compatibility with Darwin.

Soo polite 😄 It sounds good to me.

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I approve of these bugfixes being backported.

@flokli flokli merged commit e46f456 into NixOS:release-20.03 Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants