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

FIX: Call rustdoc test with the correct cfg flags of a package. #3242

Merged
merged 4 commits into from
Nov 2, 2016
Merged

FIX: Call rustdoc test with the correct cfg flags of a package. #3242

merged 4 commits into from
Nov 2, 2016

Conversation

jhbabon
Copy link
Contributor

@jhbabon jhbabon commented Oct 31, 2016

There was a situation in which if you you had a lib that depends on a package with features, whenever you ran the tests for the package the rustdoc test call was failing because it was called with the root cfg flags, not the package cfg flags.

This fix solves the issue by keeping track of the cfg flags per package, so the rustdoc command will be generated with the correct cfg flags.

Closes #3172

There was a situation in which if you you had a lib that depends
on a package with features, whenever you ran the tests for the
package the `rustdoc test` call was failing because rustdoc
was called with the root cfg flags, not the package cfg flags.

This fix solves the issue by keeping track of the cfg flags
per package, so the rustdoc command will be generated with
the correct cfg flags.
@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@TeXitoi
Copy link

TeXitoi commented Oct 31, 2016

Great! Thanks.

cx.compilation.cfgs.entry(unit.pkg.package_id().clone())
.or_insert(HashSet::new())
.insert(format!("feature=\"{}\"", feat));
}
Copy link

Choose a reason for hiding this comment

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

You can call entry just once. Something like that should work.

cx.compilation.cfgs.entry(unit.pkg.package_id().clone())
    .or_insert(HashSet::new())
    .extend(feats.iter().map(|feat| format!("feature=\"{}\"", feat));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TeXitoi that looks better, I'll check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TeXitoi change done here fd8274a

@jhbabon
Copy link
Contributor Author

jhbabon commented Nov 1, 2016

I really don't know why the tests in travis are failing. It doesn't seem related to my changes, or it is?

Also, how can I make the tests better so the pass in appveyor? Looks like the windows version is setting extra " chars and the string I put in my test is not valid.

execs().with_status(0)
.with_stderr_contains("\
[DOCTEST] a
[RUNNING] `rustdoc --test [..]--cfg feature=\\\"serde_codegen\\\"[..]`"));
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah the escaping here is subtly different on Windows (different shell), so it's fine to just match the word serde_codegen being passed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton sounds good. I'll change it.

@alexcrichton
Copy link
Member

Oh it looks like Travis is failing when trying to render https://github.com/rust-lang/cargo/blob/master/src/doc/machine-readable-output.md. Could you actually modify that file as well to change the code blocks to be tagged with the text language instead of no language at all?

The code here looks great, thanks @jhbabon!

In Windows the rustdoc test output sets more double quotes, so
the test doesn't pass. We need to relax the test so it pass in
*NIX and Windows environments.
Travis fails when running `make doc` because of this file.
This should fix the issue.
@jhbabon
Copy link
Contributor Author

jhbabon commented Nov 1, 2016

@alexcrichton changes done!

@alexcrichton
Copy link
Member

@bors: r+

Thanks @jhbabon!

@bors
Copy link
Contributor

bors commented Nov 1, 2016

📌 Commit 665aa7f has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 1, 2016

⌛ Testing commit 665aa7f with merge eca9e15...

bors added a commit that referenced this pull request Nov 1, 2016
FIX: Call rustdoc test with the correct cfg flags of a package.

There was a situation in which if you you had a lib that depends on a package with features, whenever you ran the tests for the package the `rustdoc test` call was failing because it was called with the root `cfg` flags, not the package `cfg` flags.

This fix solves the issue by keeping track of the `cfg` flags per package, so the `rustdoc` command will be generated with the correct `cfg` flags.

Closes #3172
@bors
Copy link
Contributor

bors commented Nov 2, 2016

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: alexcrichton
Pushing eca9e15 to master...

@bors bors merged commit 665aa7f into rust-lang:master Nov 2, 2016
@jhbabon jhbabon deleted the cfgs-flags-per-package branch November 2, 2016 09:52
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.

Bad call to rustdoc when cargo test --package feature when using features
5 participants