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

clean up rust.md docs #35587

Merged
merged 2 commits into from Mar 28, 2018
Merged

clean up rust.md docs #35587

merged 2 commits into from Mar 28, 2018

Conversation

vitiral
Copy link
Contributor

@vitiral vitiral commented Feb 25, 2018

Motivation for this change

I am a NixOS newbie and had a bit of difficulty getting rust setup on my box because I did not have the GNU build dependencies installed.

Things done
  • Added docs when setting up a rust environment.

  • Fixed incorrect nix-build command after setting up with carnix

  • Fits CONTRIBUTING.md although I did not open an issue for a small documentation change

@vitiral
Copy link
Contributor Author

vitiral commented Feb 25, 2018

I tested on the following environment:

 - system: `"x86_64-linux"`                                                                                                                                                                                       
 - host os: `Linux 4.14.20, NixOS, 18.03pre129076.831ef4756e3 (Impala)`                                                                                                                                           
 - multi-user?: `yes`                                                                                                                                                                                             
 - sandbox: `no`                                                                                                                                                                                                  
 - version: `nix-env (Nix) 1.11.16`                                                                                                                                                                               
 - channels(root): `"nixos-18.03pre129076.831ef4756e3"`                                                                                                                                                           
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs` 

I am on unstable, but I don't believe carnix is even on the 17.09 stable channel.

@vitiral vitiral changed the title add common gcc rust dependencies add common gcc rust dependencies and fix carnix command Feb 25, 2018
@vitiral vitiral changed the title add common gcc rust dependencies and fix carnix command clean up rust.md docs Mar 8, 2018
@vitiral
Copy link
Contributor Author

vitiral commented Mar 8, 2018

I added a fix for #36542 as well, as nix-shell is a pretty standard need for developers.

jtojnar
jtojnar previously requested changes Mar 9, 2018
@@ -261,6 +264,46 @@ general. A number of other parameters can be overridden:
};
```

### Setting Up `nix-env`
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean nix-shell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching that.

];

# Set Relevant ENV Variables
LIBRARY_PATH = "${xorg.libX11}/lib:${xorg.libXtst}/lib";
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, you're right -- it's not. I just did a cargo clean+build without it and it worked

@vitiral
Copy link
Contributor Author

vitiral commented Mar 9, 2018

@jtojnar thanks for the review, should be good to go now

];

# Set Relevant ENV Variables
PKG_CONFIG_PATH = "${openssl}/lib/pkgconfig";
Copy link
Contributor

Choose a reason for hiding this comment

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

And neither should this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm frankly shocked. I arrived at the need of environment variables through a highly technical process of trial and error (/sarcasm). I swear this didn't work before... but now it totally does.

So... I would like to have some environment variable example. I'm going to do RUST_BACKTRACE since that is a nice to have.

@vitiral
Copy link
Contributor Author

vitiral commented Mar 9, 2018

okay, should be good now :)

@vitiral
Copy link
Contributor Author

vitiral commented Mar 9, 2018

okay, so for anyone reading this (and maybe a future PR), to install things like cargo install cargo-edit (which has a dependency on openssl) when using plain NixOS itself I had to do:

  environment.variables = {
    PKG_CONFIG_PATH = [
      "${pkgs.openssl.dev}/lib/pkgconfig"
    ];
  };

Note: this has the effect of inserting the custom ones onto the default $PKG_CONFIG_PATH.

I think I just got confused between nix-shell and plain NixOS

@vitiral
Copy link
Contributor Author

vitiral commented Mar 27, 2018

@jtojnar ping, just wondering what else is necessary to move this along.

@jtojnar
Copy link
Contributor

jtojnar commented Mar 27, 2018

@vitiral A go-ahead from someone who maintains Rust in nixpkgs would be great.

@vitiral
Copy link
Contributor Author

vitiral commented Mar 27, 2018

From a cursory glance at PR's it looks like @P-E-Meunier and @dywedir might be good candidates for review?

Can one of you guys take a look? Thanks!

Copy link
Member

@globin globin left a comment

Choose a reason for hiding this comment

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

LGTM except for the typo

@@ -261,6 +264,43 @@ general. A number of other parameters can be overridden:
};
```

### Setting Up `nix-shell`
Oftentimes you want to develop code using `nix-env`. Using the example above,
Copy link
Member

Choose a reason for hiding this comment

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

nix-env -> nix-shell?

@globin globin dismissed jtojnar’s stale review March 27, 2018 18:46

stale, ack'd in the meantime

- Add example for setting up nix-shell, improve rust docs
- Rust docs: add gcc rust dependencies and fix carnix commands
- Fix a typo with the carnix command.
@vitiral
Copy link
Contributor Author

vitiral commented Mar 27, 2018

rebased, squashed and fixed review comment

@vitiral
Copy link
Contributor Author

vitiral commented Mar 27, 2018

@globin thanks!

@@ -16,6 +16,9 @@ cargo
into the `environment.systemPackages` or bring them into
scope with `nix-shell -p rustc cargo`.

> You may also want the following for building crates with C dependencies:
> `binutils gcc gnumake`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think gcc and binutils are always included in the buildInputs in a nix expressions, so this isn't actually necessary. Do you know an example of a crate that requires make (gnumake)?

Copy link
Contributor Author

@vitiral vitiral Mar 27, 2018

Choose a reason for hiding this comment

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

when I first started using NixOS I couldn't build hardly anything (edit: with C dependencies). Adding these to my configuration.nix allowed me to build things.

I'm not 100% sure gnumake is required but I included it anyway. Can totally remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll note that they should add these to environment.systemPackages in configuration.nix.

```

Crates that have the above dependencies will still compile.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's indeed a very good point to add to the documentation.

However, this is not what I usually do. Your code here would compile crate hello and put its binaries in the scope of the shell. I wonder why you would do that.

If it helps, my usual default.nix uses the builds from rustup (mostly because they're more current), even though this doesn't work on MacOS:

with import <nixpkgs> {};

let src = fetchFromGitHub {
      owner = "mozilla";
      repo = "nixpkgs-mozilla";
      rev = "42a0926f2f36cac2da53782259948ba071b6c6c5";
      sha256 = "1r2jglgl9k881byv1kc3rdda2lzaarvb0xn7nx3q0b3h25apjff5";
   };
in
with import "${src.out}/rust-overlay.nix" pkgs pkgs;

stdenv.mkDerivation {
  name = "rust-env";
  buildInputs = [ rustChannels.stable.rust ];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option (MacOS-compatible, but you won't get the nightly builds) is simply:

with import <nixpkgs> {};

stdenv.mkDerivation {
  name = "rust-env";
  buildInputs = [ rust ];
}

Copy link
Contributor Author

@vitiral vitiral Mar 27, 2018

Choose a reason for hiding this comment

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

So I think there is some confusion here...

I wanted a default.nix which can build the package (generated from carnix) and simultaneously allows me to use nix-shell. I don't think what you posted solves that problem, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sorry about that. I usually have a default.nix and a shell.nix, but what you wrote is another solution.
Have you tested though, that RUST_BACKTRACE is actually set in the resulting shell? I don't remember that any variable besides the standard ones is inherited in this way, feel free to update build-rust-crate.nix if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... it doesn't seem to work. I swear I tested it -- but I was also simultaneously testing configuration.nix. I probably set it there and didn't use --pure and got confused :(

I think this all goes back to the core point: I should use stdenv.mkDerivation instead for shell right? I will get right on that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there is a test phase in standard derivations, but I've not implemented that yet in build-rust-crate.nix. I suppose I could work on that some day, but not right now. Meanwhile, you could try to add something in build-rust-crate.nix to inherit all extra variables one might set in an overrideCrate, such as RUST_BACKTRACE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I will just rework the docs noting that those things are not yet supported for nix-shell and how to use stdenv.mkDerivation instead.

nix-shell still seems magical to me -- how does it know what things are "setup time" vs "build time" (rhetorical)? Other newbies will probably have similar questions, so I think it is a good place to admit our faults :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I didn't remember this was an issue, so PLEASE add it to the current documentation: a derivation has phases, documented in https://nixos.org/nixpkgs/manual/#sec-stdenv-phases.

Nix-build sets up the environment by downloading all the buildInputs/propagatedBuildInputs, and then runs all the phases, while nix-shell only sets up the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. I will reference the issue I just opened: #37945

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: your example of buildInputs = [ rust ] gives me this:

error: cannot coerce a set to a string, at /nix/store/y68vv1sxb24yykccac566v4xqklnzfa2-nixos-18.03pre130932.cc4677c36ee/nixos/pkgs/stdenv/generic/make-derivation.nix:148:11
(use '--show-trace' to show detailed location information)

changing to rustc cargo works.

@P-E-Meunier
Copy link
Contributor

Hi! Any documentation effort on NixOS needs to be welcomed with the highest honours! So first of all, thanks a lot! I'm glad this part of the documentation is finally being tested by new users.

I'd like to see my comments above addressed, I'm willing to review them again as soon as I can.

@P-E-Meunier
Copy link
Contributor

Question for @edolstra @Mic92 @grahamc: couldn't we have a bot to choose reviewers automatically and speed this up? This is especially important for documentation.

@vitiral
Copy link
Contributor Author

vitiral commented Mar 27, 2018

@P-E-Meunier okay, I did a bit of a refactor trying to distill your feedback. Let me know if there is anything else!

@Mic92
Copy link
Member

Mic92 commented Mar 28, 2018

@P-E-Meunier for package reviews that would not work. Because not everybody has time every day. Only because a bot pings me would not make me merge things faster. Pinging maintainers of packages is something which will be added to borg in near future I think.
For documentation the situation is unfortunate.

@Mic92 Mic92 merged commit f7342a3 into NixOS:master Mar 28, 2018
Mic92 pushed a commit that referenced this pull request Mar 28, 2018
- Add example for setting up nix-shell, improve rust docs
- Rust docs: add gcc rust dependencies and fix carnix commands
- Fix a typo with the carnix command.

(cherry picked from commit f7342a3)
@Mic92
Copy link
Member

Mic92 commented Mar 28, 2018

also ported to 18.03: 54c76d5

@P-E-Meunier
Copy link
Contributor

@Mic92 : indeed, I'm not sure I want a bot that pings nixpkgs maintainers, but on this particular PR I'd have liked to be pinged earlier, rather than manually by the author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants