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

Can't build ripgrep with cargo nightly #4287

Closed
est31 opened this issue Jul 17, 2017 · 7 comments · Fixed by #4288
Closed

Can't build ripgrep with cargo nightly #4287

est31 opened this issue Jul 17, 2017 · 7 comments · Fixed by #4288
Assignees

Comments

@est31
Copy link
Member

est31 commented Jul 17, 2017

Doing this on nightly:

$ git clone https://github.com/BurntSushi/ripgrep
$ cd ripgrep
$ cargo build

Gives you an error:

error: unable to get packages from source
Caused by:
  failed to parse manifest at `~/.cargo/registry/src/github.com-1ecc6299db9ec823/bytecount-0.1.6/Cargo.toml`

Caused by:
  can't find `bench` bench, specify bench.path

On beta everything is fine.

cc @BurntSushi @llogiq

@est31
Copy link
Member Author

est31 commented Jul 17, 2017

Using bisect-rust I managed to narrow down the regression to this commit range (rust PR rust-lang/rust#43207):
eb6cf01...f709c35

cc @matklad

@llogiq
Copy link
Contributor

llogiq commented Jul 17, 2017

Earlier rustcs looked for bench.rs in the src directory, current search in benches. Unfortunately I forgot to publish when I changed this, and I'm travelling currently.

When I arrive, I'll publish a new bytecount version. Sorry for the inconvenience.

@matklad matklad self-assigned this Jul 17, 2017
@matklad
Copy link
Member

matklad commented Jul 17, 2017

Huh, turns out the actual cause is #3947 from April, and #4259 only exposed it.

In #3947 we've intentionally removed src/bench.rs from the set of conventions. In #4259 we actually start checking that if some target is present in Cargo.toml, then it actually exists on the hard drive. I wonder if this is the correct thing to do...

@alexcrichton, a question: suppose we have a target in Cargo.toml with an explicit or an implicit path, which does not exist. Should we bail with an error when we parse the manifest, or should we wait until we actually compile the target? The former is in general more user-friendly, but the latter behavior might sometimes be useful if, for example, the target is generated by build.rs or if you exclude the target from uploading on crates.io.

The current behavior is to evaluate explicit paths lazily, but bail early for implicit paths. That is, if you have [[bench]] name=foo and no benches/foo.rs, we error when parsing the manifest. If you add path = "some/path.rs", we'll wait until cargo bench to show an error.

@matklad
Copy link
Member

matklad commented Jul 17, 2017

So we need to fix it in Cargo somehow, because bytecount is present in dependencies of various projects, including ripgrep and xi-rope.

Option 1: allow "src/bench.rs" with a warning. This basically tonnes down #3947 (when accessing impact of that PR, I've search for [test] name = "test", but forgot to search for bench as well TT).

Option 2: restore the behavior of warning about missing targets only during compilation of said targets. This could be implemented by fallback to some default path, like benches/{bench-name}.rs. This is probably the easiest approach, but I don't feel super excited about it, because if we move forward with allowing benches/{bench-name}/main.rs as well, then it won't be obvious which default path we should pick.

Option 3: use the same trick for missing targets as we do for warnings: show error only if you are compiling the crate itself, and hide errors for dependencies.

I find the first option most compelling, because it's the simplest one, although it seems like a funny step backwards from #3947.

@matklad
Copy link
Member

matklad commented Jul 17, 2017

The #4288 PR implements the first option.

bors added a commit that referenced this issue Jul 17, 2017
Allow `src/bench.rs` as a benchmark for legacy reasons

closes #4287
cc #3947
@est31
Copy link
Member Author

est31 commented Jul 17, 2017

Thanks @matklad for fixing!

@matklad
Copy link
Member

matklad commented Jul 17, 2017

Thank you @est31 for bisecting and finding out who's to blame :) It made fixing the issue so much easier!

yamafaktory added a commit to yamafaktory/jql that referenced this issue Apr 24, 2020
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 a pull request may close this issue.

3 participants