buildRustCrate: don't sort link flags

[symphorien]

Linkage order is significant and sorting can result in link errors.

This was tested (and the corresponding issue was discovered) by building nix-du with crate2nix.

nix-review reports only one compilation failure which seems unrelated (it's not a linker error)

builder for '/nix/store/ap83imyisrrq33caq1l86hf6ldsxbfy3-rust_url-1.6.0.drv' failed with exit code 1; last 10 log lines:
      |      -- lifetime `'a` defined here
  260 |     fn as_mut_string(&mut self) -> &mut String { &mut self.url.serialization }
  261 |     fn finish(self) -> &'a mut ::Url { self.url }
      |                                        ^^^^^^^^ - here, drop of `self` needs exclusive access to `*self.url`, because the type `UrlQuery<'_>` implements the `Drop` trait
      |                                        |
      |                                        returning this value requires that `*self.url` is borrowed for `'a`
  
  error: aborting due to previous error
  
  For more information about this error, try `rustc --explain E0713`.
cannot build derivation '/nix/store/xklj28hy7fx8rhrdx8dqyyzqg7g323hf-rust_serde_urlencoded-0.5.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/zl4qhhdm4ndwgjjdlh5x9fcgdz7yvx8z-rust_reqwest-0.9.5.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/dnywhrwcwjisfs16ns49sk23naq8wvnz-rust_cargo-download-0.1.2.drv': 3 dependencies couldn't be built

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.

[andir]

@symphorien thanks! That looks very good. Can you craft a case where we can simulate the error so we can add it to the buildRustCrateTests?

[symphorien]

maybe I can switch nix-du to crate2nix in nixpkgs instead of crafting an artificial example. What do you think ?

[andir]

On 14:57 25.03.20, symphorien wrote: maybe I can switch nix-du to crate2nix in nixpkgs instead of crafting an artificial example. What do you think ?

That might work now but might get lost over time. At some point we might not have the actual test case anymore - if nix-du changes significantly. The idea of `buildRustCrate` is to have a very quick feedback loop on the core pieces and those that have biten us in the past. I like to add regression tests. I am happy to have a look at how we can add those tests. Can you post the branch/changes you did do nix-du to trigger the error and the exact error messages you received?

[symphorien]

Here is a self contained repo which reproduces the bug. https://github.com/symphorien/link_order

I don't really know where to put it in nixpkgs.

[andir]

On 08:58 26.03.20, symphorien wrote: Here is a self contained repo which reproduces the bug. https://github.com/symphorien/link_order I don't really know where to put it in nixpkgs.

Thanks!. I saw the example and it make somewhat sense. I am yet not entirely sure I understand the issue that is being constructed there. I guess it is the order in which they are registered during the build.rs execution? That order must match the order symbols are being looked up during runtime/linktime? We have a bunch of buildRustCrate tests in `pkgs/build-support/rust/build-rust-crate/tests`. I tried to make it easy to construct these kinds of tests in there. I am happy to assist or take the first blow on it. I just don't want to postpone testing this as otherwise it will never happen :) (Speaking of experience). I'll be available for most of the weekend via IRC/Twitter/… if you want to chat about a potential implemention of the test.

[symphorien]

Thanks!. I saw the example and it make somewhat sense. I am yet not
entirely sure I understand the issue that is being constructed there. I
guess it is the order in which they are registered during the build.rs
execution? That order must match the order symbols are being looked up
during runtime/linktime?

It a crate which must be linked with -l static=a -l b and will fail if it is linked with -l b -l static=a.
The order of linking is inherited from the order of the flags output by build.rs.

We have a bunch of buildRustCrate tests in
pkgs/build-support/rust/build-rust-crate/tests. I tried to make it
easy to construct these kinds of tests in there. I am happy to assist or
take the first blow on it. I just don't want to postpone testing this as
otherwise it will never happen :) (Speaking of experience).

Should I copy my full example crate in nixpkgs or fetch it ?

[symphorien]

Added a regression test.

[andir]

On 06:28 28.03.20, symphorien wrote: Added a regression test.

Nice! That looks good. I picked that test to master (locally) and verified that it does indeed fail there. When applied on this branch it works as advertised. Thank you for puttin in the effort! I know it isn't straight forward. We lack documentation and comments on the test "framework". One last thing: can you retitle the last commit as below?

buildRustCrateTests: add regression test for link order

(or similar) Otherwise it looks good and I am ready to merge that in :)

[symphorien]

done

[symphorien]

Also, please backport to 20.03, if you don't mind.

[andir]

I will open a backport PR for the changes to buildRustCrate that we merged in the last few days/weeks. Those have all been non-destructive improvements/bug fixes that should be fine for inclusion.

I'll wait for #83488 to land as that seems like a good additional candidate. That should hopefully be some time this weekend. Feel free to poke me if it didn't happen.