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

Add support for Rust / Cargo packaging #7501

Merged
merged 15 commits into from
May 12, 2015
Merged

Conversation

wizeman
Copy link
Member

@wizeman wizeman commented Apr 21, 2015

As I mentioned in a comment in #4568, I continued working on Rust packaging starting from where @madjar left off.

This took me a lot longer and it was a lot harder and hackier than I thought. It's not in a completely finished and polished state, but at least it seems to work now. It allows Nix to build cargo'ed Rust programs with a minimum of fuss, including cargo itself, and it should work in a deterministic way.

Note that the fetch-cargo-deps script is probably not very robust yet with respect to git checkouts.
Specifically, I have not tested building a Rust program that has a git dependency whose repository has submodules (I would be surprised if this worked without some additional fixes). If anyone has an example of such a program, please let me know and I will fix the script.

As @madjar had mentioned, it would be nice to add a nix-prefetch-cargo script and documentation, but even in this state I think it would already be useful to anyone who wants to build Rust programs.

A big thanks to @madjar for his initial work!

wizeman and others added 3 commits April 21, 2015 19:46
This is useful when `leaveDotGit = true` and some other derivation
expects some branch name to exist.

Previously, `nix-prefetch-git` always created a branch with a
hard-coded name (`fetchgit`).
@jagajaga
Copy link
Member

Looks cool! Going to look deeper later. Thank you!

Also disable check phase in cargo as there are lots of failures (some
probably due to trying to access the network).
@madjar
Copy link
Member

madjar commented Apr 22, 2015

Awesome, thanks for the hard work! There's indeed some very hackish things in there, but that's good enough for now :)

We'll also need to add a paragraph in the documentation about how to package a rust package. I think we should invoke some nixpkgs guru for review, since this is very big.

By the way, what do you think about dropping the rust snapshot after 1.0 is released?

It turns out that `cargo`, with respect to registry dependencies, was
ignoring the package versions locked in `Cargo.lock` because we changed
the registry index URL.

Therefore, every time `rustRegistry` would be updated, we'd always try
to use the latest version available for every dependency and as a result
the deps' SHA256 hashes would almost always have to be changed.

To fix this, now we do a string substitution in `Cargo.lock` of the
`crates.io` registry URL with our URL. This should be safe because our
registry is just a copy of the `crates.io` registry at a certain point
in time.

Since now we don't always use the latest version of every dependency,
the build of `cargo` actually started to fail because two of the
dependencies specified in its `Cargo.lock` file have build failures.

To fix the latter problem, I've added a `cargoUpdateHook` variable that
gets ran both when fetching dependencies and just before building the
program. The purpose of `cargoUpdateHook` is to do any ad-hoc updating
of dependencies necessary to get the package to build. The use of the
'--precise' flag is needed so that cargo doesn't try to fetch an even
newer version whenever `rustRegistry` is updated (and therefore have to
change depsSha256 as a consequence).
@wizeman
Copy link
Member Author

wizeman commented Apr 23, 2015

@madjar Which rust snapshot do you mean? I thought that the rustc snapshot is needed to build rustc itself (in the same way that cargoSnapshot is needed to build cargo). So I don't see how we can drop it?

@wizeman
Copy link
Member Author

wizeman commented Apr 23, 2015

Note that I've just noticed that there was a bug and I've just added a fix for it. It turns out that Cargo.lock was actually being ignored, causing depsSha256 to have to be changed whenever rustRegistry was updated. See the bug fix's commit comment for more details.

I've also found a way to get rid of the /proc/self/cwd (and associated symlink) hack, however it has a drawback: it depends on a few more internal details of how cargo works (specifically, it will require exact knowledge of how cargo hashes the registry index URL, and also a couple more details about the directory structure that cargo fetch creates).

However, the major benefit is that it would make buildRustPackage work on platforms other than Linux, due to getting rid of this platform-specific hack. It should also make the build a bit faster, because the registry index directory structure wouldn't have to be recreated by cargo when we do cargo build --release.

I assume this is a worthwhile tradeoff, so if nobody objects I will start working on it tomorrow.

@madjar
Copy link
Member

madjar commented Apr 23, 2015

@wizeman Oh, sorry, I meant the rustcMaster.

What upstream change would be required to make it cleaner ?

Instead, move that code into buildRustPackage.

The setup hook was only doing part of the work anyway, and having it in
a separate place was obscuring what was really going on.
... in a more generic way.

With this commit, if you need to patch a registry package to make it
work with Nix, you just need to add a script to patch-registry-deps
in the same style as the `pkg-config` script.
This makes buildRustPackage portable to non-Linux platforms.

Additionally, now we also save the `Cargo.lock` file into the fetch output, so
that we don't have to run $cargoUpdateHook again just before building.
For some reason, `cargo` doesn't like git repositories downloaded with
`fetchgit`. It will sometimes fail with the error message "Target OID for
the reference doesn't exist on the repository".

To workaround this, we'll just have to create a new git repo from
scratch...
@wizeman
Copy link
Member Author

wizeman commented Apr 23, 2015

@madjar I think we shouldn't delete rustcMaster, as it will be required for people who want to test the master release channel (also, at the moment some packages still depend on rustcMaster: racerRust and cargo, and I'm not sure if they will support the stable channel before 1.0 is released).

Regarding making this cleaner, I opened rust-lang/cargo#1334 a few months ago, but it was closed and I was told to simply rely on cargo fetch...

@wizeman
Copy link
Member Author

wizeman commented Apr 23, 2015

Ok, I've fixed a new bug that I found, where the local registry was being ignored because cargo sometimes doesn't seem to like repositories downloaded by fetchgit, instead giving out an error that it wasn't able to update.

I'm not sure why this happens sometimes, I've tried to look into it a bit but I just don't know much about git internals and I wasn't looking forward to start digging into the libgit2 code. After I spent quite some time trying to find a workaround, I found out that if I locally create a new, from scratch, copy of the repository downloaded with fetchgit, it will work fine, so even though this is suboptimal, at least it works reliably for now. Hopefully in the future cargo/libgit2 will get fixed and we'll just be able to use fetchgit directly.

On the other hand, I finally got rid of the /proc/self/cwd hack and I'm quite happy with the results. This means that buildRustPackage will now work on non-Linux architectures.

I also did a few cleanups and I added a bunch of comments explaining what is being done, especially in the fetch-cargo-deps script.

Since I was on a roll, I also factored out the patching of the pkg-config registry dependency into a more general patching facility, so that in the future we have a global place where we can easily patch dependencies (instead of duplicating all of these patches in each derivation of a Rust program).

@wizeman wizeman added the 0.kind: enhancement Add something new label Apr 23, 2015
@wmertens
Copy link
Contributor

Very nice!

Question though, did you involve any Rustaceans in your struggles with
cargo? I'd think they'd be interested in helping with this effort...

On Fri, Apr 24, 2015 at 1:22 AM Ricardo M. Correia notifications@github.com
wrote:

Ok, I've fixed a new bug that I found, where the local registry was being
ignored because cargo sometimes doesn't seem to like repositories
downloaded by fetchgit, instead giving out an error that it wasn't able
to update.

I'm not sure why this happens sometimes, I've tried to look into it a bit
but I just don't know much about git internals and I wasn't looking forward
to start digging into the libgit2 code. After I spent quite some time
trying to find a workaround, I found out that if I locally create a new,
from scratch, copy of the repository downloaded with fetchgit, it will
work fine, so even though this is suboptimal, at least it works reliably
for now. Hopefully in the future cargo/libgit2 will get fixed and we'll
just be able to use fetchgit directly.

On the other hand, I finally got rid of the /proc/self/cwd hack and I'm
quite happy with the results. This means that buildRustPackage is now
portable to non-Linux architectures.

I also did a few cleanups and I added a bunch of comments explaining what
is being done, especially in the fetch-cargo-deps script.

Since I was on a roll, I also factored out the patching of the pkg-config
registry dependency into a more general patching facility, so that in the
future we have a global place where we can easily patch dependencies
(instead of duplicating all of these patches in each derivation of a Rust
program).


Reply to this email directly or view it on GitHub
#7501 (comment).

@wizeman
Copy link
Member Author

wizeman commented Apr 27, 2015

Question though, did you involve any Rustaceans in your struggles with
cargo? I'd think they'd be interested in helping with this effort...

I opened cargo issue 1330 and was then told to open another issue (cargo issue 1334), but in the end there didn't seem to be any interest in what I was proposing...

At this point I'm not even sure what I'd like to propose anymore. Although I'm tempted to want a more traditional approach to packaging Rust libraries, such as what we do for other languages, their suggested use of cargo fetch also has its advantages, such as less maintenance burden for distro packagers and more guaranteed compatibility between Rust libraries and Rust programs.

The only major worry I have with the way cargo bundles Rust libraries is the way cargo / rustc wants to statically link almost all library dependencies.

Specifically, I am concerned about the difficulty of ensuring we are not shipping any Rust program with a statically linked library with known security vulnerabilities.

But I'm also thinking that assuming most Rust programs will depend on libraries that are in the crates.io registry, it shouldn't be too hard to check that we are not shipping any vulnerable libraries. Especially given that all libraries in the registry are versioned, we could just maintain a registry of security-vulnerable library versions, and during the build of a package we just make sure that "cargo fetch" didn't download any of these vulnerable versions...

It would be more difficult to do this for git-based dependencies, though...

@jagajaga
Copy link
Member

jagajaga commented May 8, 2015

When are you going to merge this?

wizeman added a commit that referenced this pull request May 12, 2015
Add support for Rust / Cargo packaging
@wizeman wizeman merged commit 755df64 into NixOS:master May 12, 2015
@wizeman
Copy link
Member Author

wizeman commented May 12, 2015

Merged.

@madjar
Copy link
Member

madjar commented May 12, 2015

Congratulations!

@wizeman wizeman deleted the u/upd-rust branch June 4, 2015 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants