NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.4k stars 14.35k forks source link

buildRustCrate: erroneously builds binary as library #64313

Open dnaq opened 5 years ago

dnaq commented 5 years ago

A binary crate that contains a source file that has the same name as the crate itself will erroneously be built as a library, even though it contains a main.rs. If that source file has dependencies on the actual crate the build will fail.

cargo new --bin foo
cat > src/main.rs << EOF
mod foo;
mod bar;
fn main() { }
EOF

cat > src/foo.rs << EOF
use bar::*;
EOF

touch src/bar.rs

cargo generate-nixfile --standalone --src ./.
nix-build Cargo.nix -A foo
Building src/foo.rs (foo)
Running rustc --crate-name foo src/foo.rs --crate-type lib -C opt-level=3 -C codegen-units=8 --edition 2018 -C metadata=cb022a3647 -C extra-filename=-cb022a3647 --out-dir target/lib --emit=dep-info,link -L dependency=target/deps --cap-lints allow --color always
error[E0432]: unresolved import `bar`
 --> src/foo.rs:1:5
  |
1 | use bar::*;
  |     ^^^ use of undeclared type or module `bar`

error: aborting due to previous error

The following lines seem to be the issue, and commenting them out makes the build work, however I'm not sure if that would break anything else.

https://github.com/NixOS/nixpkgs/blob/c3cc7034e2562b110cd12192e0f390ad25cb5dbe/pkgs/build-support/rust/build-rust-crate/build-crate.nix#L88-L89

@P-E-Meunier would you like to take a look at this

P-E-Meunier commented 5 years ago

I believe @andir added these lines, according to the blame. I've never seen that case, and I don't remember using that particular heuristic to do this, but maybe he has.

According to the Cargo documentation, it seems wrong (i.e. you're right to want to delete these two lines): https://doc.rust-lang.org/1.31.0/cargo/reference/manifest.html#the-project-layout

dnaq commented 5 years ago

I believe the heuristic is wrong as well. I also tried building a library crate that didn’t have a lib.rs using cargo with the source file named the same as the library. Cargo refused to build it that way, so I believe we should remove those lines. Still it might be nice to test to make sure that nothing else breaks.

andir commented 5 years ago

IIRC I added each of those test cases as I was observing build errors with them not just because I thought so. It could be that upstream cargo no longer does this but older crates might have done it. I vaguely remember cargo having an older (legacy?) build path and a newer one. Maybe that was cleaned up?

@dnaq do you mind opening a PR that updates build-crate.nix and changes the corresponding tests in pkgs/build-support/rust/build-rust-crate/test?

dnaq commented 5 years ago

@andir I've started working on it, but when running nox-review wip it seems like the crate c_vec fails to build. Looking at the source of c_vec it seems like the old versions used paths like src/c_vec.rs, whereas the new versions (same major release) uses src/lib.rs instead. See https://github.com/GuillaumeGomez/c_vec-rs/commit/80ce3504add4283c6bc444f168865c3cd800056a.

The only thing that depends on c_vec in nixpkgs is way-cooler, but I'm not sure on how to regenerate the nix-files for it. We should be able to bump its dependency version somehow.

dnaq commented 5 years ago
➜  pkgs git:(build-rust-crate) ✗ rg c_vec
applications/window-managers/way-cooler/way-cooler.nix
19:  deps.c_vec."1.2.1" = {};
21:    c_vec = "1.2.1";

applications/window-managers/way-cooler/crates-io.nix
110:# c_vec-1.2.1
112:  crates.c_vec."1.2.1" = deps: { features?(features_.c_vec."1.2.1" deps {}) }: buildRustCrate {
113:    crateName = "c_vec";
118:  features_.c_vec."1.2.1" = deps: f: updateFeatures f (rec {
119:    c_vec."1.2.1".default = (f.c_vec."1.2.1".default or true);
134:      (crates."c_vec"."${deps."cairo_rs"."0.2.0"."c_vec"}" deps)
149:    c_vec."${deps.cairo_rs."0.2.0".c_vec}".default = true;
191:    (features_.c_vec."${deps."cairo_rs"."0.2.0"."c_vec"}" deps)
dnaq commented 5 years ago

So, researching the issue some more it seems like if there's a

[lib]
name = "c_vec"

Then cargo will allow a source file with the name src/c_vec, but if that line is missing from the toml then we shouldn't allow it.

So my fix won't work in general, but the current behaviour is wrong as well.

dnaq commented 5 years ago

Seems like cargo emits the following warning when trying to build the old version of c_vec.

warning: path `/home/dna/t/c_vec-rs/src/c_vec.rs` was erroneously implicitly accepted for library `c_vec`,
please rename the file to `src/lib.rs` or set lib.path in Cargo.toml

So the patch should work, but someone needs to update the dependencies of way-cooler to avoid it breaking.

andir commented 5 years ago

I think an escape hatch to opt-in to that old behavior would be okay if it were left off by default.

On Sat, 6 Jul 2019, 17:36 dnaq, notifications@github.com wrote:

Seems like cargo emits the following warning when trying to build the old version of c_vec.

warning: path /home/dna/t/c_vec-rs/src/c_vec.rs was erroneously implicitly accepted for library c_vec, please rename the file to src/lib.rs or set lib.path in Cargo.toml

So the patch should work, but someone needs to update the dependencies of way-cooler to avoid it breaking.,

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NixOS/nixpkgs/issues/64313?email_source=notifications&email_token=AAE365HP3N3OJV2GXATTH6DP6C3ZDA5CNFSM4H56WMJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZK3WOA#issuecomment-508934968, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE365HNM475H2EXKYW4U3DP6C3ZDANCNFSM4H56WMJA .

dnaq commented 5 years ago

@andir, with the following patch to way-cooler nox-review wip succeeds as well.

diff --git a/pkgs/applications/window-managers/way-cooler/crates-io.nix b/pkgs/applications/window-managers/way-cooler/crates-io.nix
index 9dbd367a67f..4fe0004e658 100644
--- a/pkgs/applications/window-managers/way-cooler/crates-io.nix
+++ b/pkgs/applications/window-managers/way-cooler/crates-io.nix
@@ -107,16 +107,16 @@ rec {

 # end
-# c_vec-1.2.1
+# c_vec-1.3.3

-  crates.c_vec."1.2.1" = deps: { features?(features_.c_vec."1.2.1" deps {}) }: buildRustCrate {
+  crates.c_vec."1.3.3" = deps: { features?(features_.c_vec."1.3.3" deps {}) }: buildRustCrate {
     crateName = "c_vec";
-    version = "1.2.1";
+    version = "1.3.3";
     authors = [ "Guillaume Gomez <guillaume1.gomez@gmail.com>" ];
-    sha256 = "15gm72wx9kd0n51454i58rmpkmig8swghrj2440frxxi9kqg97xd";
+    sha256 = "18x52cdv2gh5lfi9z3pdcrhk56qwjsy28p5w5183rb58rv3nr94c";
   };
-  features_.c_vec."1.2.1" = deps: f: updateFeatures f (rec {
-    c_vec."1.2.1".default = (f.c_vec."1.2.1".default or true);
+  features_.c_vec."1.3.3" = deps: f: updateFeatures f (rec {
+    c_vec."1.3.3".default = (f.c_vec."1.3.3".default or true);
   }) [];

diff --git a/pkgs/applications/window-managers/way-cooler/way-cooler.nix b/pkgs/applications/window-managers/way-cooler/way-cooler.nix
index 28a327f1c13..4d8bdef577c 100644
--- a/pkgs/applications/window-managers/way-cooler/way-cooler.nix
+++ b/pkgs/applications/window-managers/way-cooler/way-cooler.nix
@@ -16,9 +16,9 @@ rec {
   deps.bitflags."0.7.0" = {};
   deps.bitflags."0.9.1" = {};
   deps.bitflags."1.0.4" = {};
-  deps.c_vec."1.2.1" = {};
+  deps.c_vec."1.3.3" = {};
   deps.cairo_rs."0.2.0" = {
-    c_vec = "1.2.1";
+    c_vec = "1.3.3";
     cairo_sys_rs = "0.4.0";
     glib = "0.3.1";
     glib_sys = "0.4.0";
dywedir commented 5 years ago

FYI way-cooler is marked as broken anyway

stale[bot] commented 4 years ago

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.