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

rust uplift for 17.03 #26970

Closed
wants to merge 10 commits into from
Closed

Conversation

garbas
Copy link
Member

@garbas garbas commented Jun 29, 2017

Motivation for this change

Rust is keeping backward compatibility in 1.x and it should be safe to uplift all rust related changes to 17.03.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

dezgeg and others added 10 commits June 29, 2017 18:39
Since cargo build became a rust package, postInstall hook was no longer
called. When the hook was reenabled in
cdd11368426380db545cad84c76e350e5e201adc, it revealed that scripts
leftover from the component based installer are no longer present.
Also updates beta, nightly, nightlyBin, and bootstrap compilers.
Also updates the registry.
Also consolidates logic between bootstrap and nightlyBin compilers.
Also contains some miscellaneous cleanups.
Also patches firefox to build with the newer cargo
bootstrapping rust requires the prior version of rust according to #rust-internals

they theorize this could be causing the build problems on i686
We saw this error when building main.rs:error: this needs a 'static lifetime or the static_in_const feature, see NixOS#35897
@garbas garbas changed the base branch from master to release-17.03 June 29, 2017 16:52
@garbas garbas requested review from the-kenny, Mic92 and qknight and removed request for the-kenny, Mic92 and qknight June 29, 2017 17:42
@garbas
Copy link
Member Author

garbas commented Jun 29, 2017

dont yet merge i'm still building locally so that everything will works

@qknight
Copy link
Member

qknight commented Jun 29, 2017

paul, aszlig and me are about to removing cargo from the nixos toolchain and create a cargo2nix tool next week. would be nice if this patch is in then as it looks promising, .... especially if it builds.

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2017

@qknight similar to #24991 ?

@bennofs
Copy link
Contributor

bennofs commented Jun 30, 2017

@qknight what's the design of cargo2nix going to be? will it fully replace cargo and call rustc manually or will it just generate derivations that then use cargo?

@qknight
Copy link
Member

qknight commented Jun 30, 2017

@Mic92 @bennofs @sjmackenzie @seitz
the plan is this: cargo creates a cargo.lock file. we write a simple rust application, called cargo2nix, which generates a cargo.lock.nix file from it.

cargo2nix internal

  1. cargo2nix additionally has a small database, a nix attribute set, where we map additional system dependencies as sqlite for packages like rusqlite which requires libsqlite3-sys ^0.8 which assumes sqlite-dev to be present.

  2. later on: cargo.lock.nix file is commited into nixpkgs alongside the respective packaged project using it. in comparison, this is what we do for yarn2nix for our nixcloud-frontend in the javascript world.

note: cargo is then only used for generating the cargo.lock.nix file and during project build never called again.

references

what has been created in https://github.com/NixOS/nixpkgs/pull/24991/files looks similar to https://github.com/nixcloud/nixcrates/blob/master/default.nix .

we are going build something like buildRustPackage which builds all single dependencies, listed in cargo.lock.nix first, then links them all with the project object into the final ELF binary. historically this would be done by cargo and is a constant source of pain in nixos. in nixcrates, for reference, we already engineered the part where the build happens without any cargo involvement. the nixcrates drawback is that semver is ignored and we are duplicating the code of dependency calculation which would be done by cargo.

i strongly encourage to use the dependency calculator of cargo and not to duplicated the code as we did in nixcrates because it avoids to have two sources of errors when building targets using cargo, on say ubuntu, and using nixcrates with nix.

todo

yet we have to check if cargo.lock already holds all the information we need to build a cargo2nix tool with. a first glimples looked promising. however, if that does not hold true, we would extend cargo with a plugin which would ouput what we required for making the cargo2nix tool.

@bennofs
Copy link
Contributor

bennofs commented Jun 30, 2017

@qknight what is the motiviation for not using the cargo tool for building?

@qknight
Copy link
Member

qknight commented Jun 30, 2017

motivation for dropping cargo

@bennofs one original motivation for nixcrates was to be able to describe dependencies in nix like this:

buildInputs = with allCrates; [ tar filetime libc xattr ]; 

one is also able to pick a specific version of a crate using nixcrates.

post mortem

turns out that cargo2nix wouldn't help in this regard and it is good that you asked this question. i have to think about this much more. one problem with cargo2nix, as with all these tools, like yarn2nix, is that we can't use nix to evaluate them but have to do that manually.

maybe we can do something with https://github.com/shlevy/nix-exec to be able to call cargo2nix from a shell and then extend the dependencies. will report back when i gave this more thought.

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2017

@qknight I really like the approach, how this is handled for ruby modules in nixpkgs.

@sjmackenzie
Copy link
Contributor

@qknight actually the original motivation was a mix of being able to describe dependencies in such a brief manner but also to facilitate incremental recompilation which is achieved by "builds all single dependencies" then "then links them all with the project object into the final ELF binary". Primarily the incremental compilation.

@qknight
Copy link
Member

qknight commented Jun 30, 2017

@sjmackenzie thanks for bringing me back on track!

@sjmackenzie
Copy link
Contributor

I'm not sure it's possible to achieve the succinct dependency declaration, it requires a Cargo.toml + Cargo.lock + Cargo.lock.nix.

In an attempt to rescue the succinct dependency declaration the default.nix could call the Cargo.lock.nix file, this way users are only exposed to this:

{ agent, edges, mods, pkgs }:

agent {
  src = ./.;
  edges = with edges; [ PrimBool ];
  mods = with mods.rs; [ ./Cargo.lock.nix ];
  osdeps = with pkgs; [];
}

Though the urgency of a functional dependency calculator is more important than aesthetics atm/imho.

@@ -12,6 +25,6 @@ fi

for PLATFORM in $PLATFORMS
do
URL="$BASEURL/rust-$VERSION-$PLATFORM.tar.gz.sha256"
URL="$BASEURL/$DATE/rust-$VERSION-$PLATFORM.tar.gz.sha256"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove the quotes here and add them on the next line.

@@ -98,15 +80,26 @@ stdenv.mkDerivation {
# https://reviews.llvm.org/rL281650
rm -vr src/test/run-pass/issue-36474.rs || true

# Disable some failing gdb tests. Try re-enabling these when gdb
# is updated past version 7.12.
rm src/test/debuginfo/basic-types-globals.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Call rm once instead.

Copy link
Contributor

@0xABAB 0xABAB left a comment

Choose a reason for hiding this comment

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

In many places the quoting needs to be fixed, but assuming it works, it looks fine.

@garbas
Copy link
Member Author

garbas commented Aug 1, 2017

i'm closing this since i will focus on things in current master since it will becomes stable soon

@garbas garbas closed this Aug 1, 2017
@garbas garbas deleted the rust-uplift-for-17.03 branch August 1, 2017 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants