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

rustdoc: don't prefer dynamic linking in doc tests #54939

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Oct 9, 2018

This is an attempt to address the regression in #54478

This may be a case where the cure is worse than the disease, at least in the short term...

cc @alexcrichton

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2018
@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 9, 2018

@bors try

@bors
Copy link
Contributor

bors commented Oct 9, 2018

⌛ Trying commit 9a76c93 with merge 40d4795...

bors added a commit that referenced this pull request Oct 9, 2018
…c-tests, r=<try>

[WIP] rustdoc: don't prefer dynamic linking in doc tests

This is an attempt to address the regression in #54478

This may be a case where the cure is worse than the disease, at least in the short term...

cc @alexcrichton
@alexcrichton
Copy link
Member

Ok I finally got around to my local machine, and it looks like this passes ./x.py test with the commit I've added to update some UI test output. Other than that let's see what crater has to say...

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 9, 2018

oh darn I clearly should have left a blank line there! 😆

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 9, 2018

@craterbot run name=rustdoc-test-static-cling start=master#0e07c4281c343e9e15a0a8fca79538ad1a8eb513 end=try#40d4795669493f1965de7c44029c69552134fe1f mode=build-and-test

@craterbot
Copy link
Collaborator

craterbot commented Oct 9, 2018

👌 Experiment rustdoc-test-static-cling 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 Oct 9, 2018
@craterbot
Copy link
Collaborator

🚧 Experiment rustdoc-test-static-cling 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 rustdoc-test-static-cling is completed!
📰 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 Oct 12, 2018
@alexcrichton
Copy link
Member

Well the good news is that this fixed 10 crates. The bad news is that it regressed 23000 crates.

@pietroalbini random selections of error logs look sort of confusing, did this run have errors in the middle perhaps?

@Mark-Simulacrum
Copy link
Member

Yes, I believe the conclusion was that the run itself errored (rather than this PR causing most of the failures).

@craterbot run name=rustdoc-test-static-cling-1 start=master#0e07c4281c343e9e15a0a8fca79538ad1a8eb513 end=try#40d4795669493f1965de7c44029c69552134fe1f mode=build-and-test

@craterbot
Copy link
Collaborator

👌 Experiment rustdoc-test-static-cling-1 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 Oct 12, 2018
@craterbot
Copy link
Collaborator

🚧 Experiment rustdoc-test-static-cling-1 is now running on agent aws-2.

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

@craterbot
Copy link
Collaborator

🎉 Experiment rustdoc-test-static-cling-1 is completed!
📰 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 Oct 16, 2018
@Mark-Simulacrum
Copy link
Member

https://crater-reports.s3.amazonaws.com/rustdoc-test-static-cling-1/try%2340d4795669493f1965de7c44029c69552134fe1f/reg/docmatic-0.1.2/log.txt
A few of these: https://crater-reports.s3.amazonaws.com/rustdoc-test-static-cling-1/try%2340d4795669493f1965de7c44029c69552134fe1f/reg/restson-0.4.0/log.txt
https://crater-reports.s3.amazonaws.com/rustdoc-test-static-cling-1/try%2340d4795669493f1965de7c44029c69552134fe1f/reg/structopt-flags-0.1.0/log.txt

Haven't tried to reproduce locally but those are the failures that look possibly non-spurious. The wrong crate/mismatched types is the only one I'm super concerned with -- but since it's one case out of ~80k crates tested it seems unlikely that it's actually a problem. As such, presuming we're not expecting a serious performance regression here, I think this is ready to go

@pnkfelix pnkfelix changed the title [WIP] rustdoc: don't prefer dynamic linking in doc tests rustdoc: don't prefer dynamic linking in doc tests Oct 16, 2018
@pnkfelix pnkfelix added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-nominated labels Oct 16, 2018
@pnkfelix
Copy link
Member Author

(I just want someone from https://www.rust-lang.org/en-US/team.html#Rustdoc-team to give a 👍 here before we go and land it...)

@pnkfelix
Copy link
Member Author

r? @QuietMisdreavus

@pnkfelix pnkfelix removed the request for review from Mark-Simulacrum October 16, 2018 21:28
@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 16, 2018
@Mark-Simulacrum Mark-Simulacrum added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 16, 2018
@QuietMisdreavus
Copy link
Member

Based on @alexcrichton's reasoning in #54478 (comment), this gets a +1 from me. It would be nice to know why it was set like that in the first place, but since it was set like that for as long as rustdoc has had doctests, i doubt we'll know for sure.

@pnkfelix
Copy link
Member Author

@bors r=QuietMisdreavus

@bors
Copy link
Contributor

bors commented Oct 17, 2018

📌 Commit cbca688 has been approved by QuietMisdreavus

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 17, 2018
@bors
Copy link
Contributor

bors commented Oct 17, 2018

⌛ Testing commit cbca688 with merge 9d7f0da...

bors added a commit that referenced this pull request Oct 17, 2018
…c-tests, r=QuietMisdreavus

rustdoc: don't prefer dynamic linking in doc tests

This is an attempt to address the regression in #54478

This may be a case where the cure is worse than the disease, at least in the short term...

cc @alexcrichton
@bors
Copy link
Contributor

bors commented Oct 17, 2018

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

@bors bors merged commit cbca688 into rust-lang:master Oct 17, 2018
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 18, 2018
bors added a commit that referenced this pull request Oct 18, 2018
[beta] Rollup backports

Merged and approved:

* #54300: Updated RELEASES.md for 1.30.0
* #54939: rustdoc: don't prefer dynamic linking in doc tests
* #54671: resolve: Scale back hard-coded extern prelude additions on 2015 edition
* #55102: resolve: Do not skip extern prelude during speculative resolution

r? @ghost
bors added a commit that referenced this pull request Feb 19, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants