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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Don't modify library path for doctests #56514

Merged
merged 1 commit into from
Feb 19, 2019
Merged

rustdoc: Don't modify library path for doctests #56514

merged 1 commit into from
Feb 19, 2019

Conversation

ollie27
Copy link
Member

@ollie27 ollie27 commented Dec 4, 2018

It shouldn't be needed anymore because doctests are no longer compiled with prefer-dynamic (since #54939).

r? @QuietMisdreavus

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2018
@ollie27 ollie27 mentioned this pull request Dec 4, 2018
Merged
@QuietMisdreavus
Copy link
Member

This feels like something that could use a crater run...

@QuietMisdreavus
Copy link
Member

Just to sate my paranoia, i'm going to try to run this through crater. First step...

@bors try

@bors
Copy link
Contributor

bors commented Dec 4, 2018

⌛ Trying commit 7dd34d505bcccad927fa1f62414ee0294d1e4c0b with merge 6eb0fd3ac58f9b16bddb9924a17c3b6c156aa468...

@bors
Copy link
Contributor

bors commented Dec 4, 2018

☀️ Test successful - status-travis
State: approved= try=True

@QuietMisdreavus
Copy link
Member

@craterbot run start=master#906deae0790bd18681b937fe9a141a3c26cf1855 end=try#6eb0fd3ac58f9b16bddb9924a17c3b6c156aa468 mode=build-and-test

@craterbot
Copy link
Collaborator

👌 Experiment pr-56514 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2018
@craterbot
Copy link
Collaborator

🚧 Experiment pr-56514 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-56514 is completed!
📊 17 regressed and 15 fixed (50140 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 8, 2018
@Mark-Simulacrum
Copy link
Member

These are the suspicious looking logs, I believe the rest are spurious:

https://crater-reports.s3.amazonaws.com/pr-56514/try%236eb0fd3ac58f9b16bddb9924a17c3b6c156aa468/reg/liquid-0.17.1/log.txt
https://crater-reports.s3.amazonaws.com/pr-56514/try%236eb0fd3ac58f9b16bddb9924a17c3b6c156aa468/reg/read-byte-slice-0.1.2/log.txt
https://crater-reports.s3.amazonaws.com/pr-56514/try%236eb0fd3ac58f9b16bddb9924a17c3b6c156aa468/reg/sparkpost-0.5.2/log.txt
https://crater-reports.s3.amazonaws.com/pr-56514/try%236eb0fd3ac58f9b16bddb9924a17c3b6c156aa468/reg/strip-ansi-escapes-0.1.0/log.txt
https://crater-reports.s3.amazonaws.com/pr-56514/try%236eb0fd3ac58f9b16bddb9924a17c3b6c156aa468/reg/warp-0.1.9/log.txt
https://crater-reports.s3.amazonaws.com/pr-56514/try%236eb0fd3ac58f9b16bddb9924a17c3b6c156aa468/reg/xsv-0.13.0/log.txt

@QuietMisdreavus
Copy link
Member

All of the legit failures look like docmatic-style tests where people run doctests from their README by calling rustdoc directly. I guess the directory they pass to rustdoc (-L target/debug/deps in most examples) has both the cfg(test) and cfg(not(test)) versions of the rlib in it, which confuses the crate loader.

However, the more i look at docmatic and skeptic reverse-deps, the more i see them everywhere on the report. I wonder if there's something else that's causing the failures, and this specific change isn't actually what's at fault.

Regardless, GitHub is now reporting a merge conflict.

@QuietMisdreavus QuietMisdreavus added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2018
@XAMPPRocky
Copy link
Member

Triage; @ollie27 Hello, have you been able to get back to this PR?

@bors
Copy link
Contributor

bors commented Jan 18, 2019

☔ The latest upstream changes (presumably #56189) made this pull request unmergeable. Please resolve the merge conflicts.

@ollie27
rustdoc: Don't modify library path for doctests
ad09d7f 
It shouldn't be needed anymore because doctests are no longer compiled with `prefer-dynamic`.
@ollie27
Copy link
Member Author

ollie27 commented Jan 25, 2019

It doesn't look like any of those crater failures are because of this PR. Technically this PR does break running rustdoc -Cprefer-dynamic --test foo.rs but I don't know why anyone would do that.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2019
@QuietMisdreavus
Copy link
Member

Rustdoc taking arbitrary -C flags is fairly new (it was only added last year), so i don't think there's much risk of anyone doing that.

I'm going to go ahead and merge this PR - it's been sitting around long enough, and the Crater run showed that it doesn't affect anyone. Sorry for letting it sit forever!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2019

📌 Commit ad09d7f has been approved by QuietMisdreavus

@bors
Copy link
Contributor

bors commented Feb 18, 2019

🌲 The tree is currently closed for pull requests below priority 160, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 18, 2019
@bors
Copy link
Contributor

bors commented Feb 19, 2019

⌛ Testing commit ad09d7f with merge fcccf06...

bors added a commit that referenced this pull request Feb 19, 2019
@bors
Auto merge of #56514 - ollie27:rustdoc_test_libdir, r=QuietMisdreavus
fcccf06 
rustdoc: Don't modify library path for doctests

It shouldn't be needed anymore because doctests are no longer compiled with `prefer-dynamic` (since #54939).

r? @QuietMisdreavus
@bors
Copy link
Contributor

bors commented Feb 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: QuietMisdreavus
Pushing fcccf06 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 19, 2019
@bors bors merged commit ad09d7f into rust-lang:master Feb 19, 2019
This was referenced Feb 19, 2019
Merged
Merged
@ollie27 ollie27 deleted the rustdoc_test_libdir branch February 24, 2019 18:26
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Mar 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@QuietMisdreavus QuietMisdreavus

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ollie27 @QuietMisdreavus @bors @craterbot @Mark-Simulacrum @XAMPPRocky @rust-highfive @Dylan-DPC-zz