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

Turn sufficiently old compatibility lints into hard errors #42136

Merged
merged 3 commits into from
Jun 1, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented May 21, 2017

It's been almost 7 months since #36894 was merged, so it's time to take the next step.

[breaking-change], needs crater run.

PRs/issues submitted to affected crates:
https://github.com/alexcrichton/ctest/pull/17
Sean1708/rusty-cheddar#55
m-r-r/helianto#3
azdle/virgil#1
rust-locale/rust-locale#24
mneumann/acyclic-network-rs#1
reem/rust-typemap#38

cc https://internals.rust-lang.org/t/moving-forward-on-forward-compatibility-lints/4204
cc #34537 #36887
Closes #36886
Closes #36888
Closes #36890
Closes #36891
Closes #36892
r? @nikomatsakis

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 22, 2017

Started crater run:

@nikomatsakis
Copy link
Contributor

Crater report: https://gist.github.com/nikomatsakis/d52d52ad9593528326deda76fb8281e0

Not yet analyzed.

Coverage

  • 9031 crates tested: 6575 working / 2086 broken / 152 regressed / 35 fixed / 183 unknown.

Regressions

  • There are 51 root regressions
  • There are 152 regressions

Root regressions, sorted by rank:

@Mark-Simulacrum
Copy link
Member

Primary problems are petgraph and typemap, though there are a few others.

root cause          lint                                      recorded cause
acyclic-network     type param defaults                       cppn-0.1.1
chrono              illegal_struct_or_enum_constant_pattern   helianto-0.1.0-beta1
elfloader32         illegal_struct_or_enum_constant_pattern   rustv-0.5.1
locale              private in public                         locale-0.2.1
mustache            private in public                         virgil-0.2.1
petgraph            type param defaults                       gluon_c-api-0.4.0
petgraph            type param defaults                       gluon_language-server-0.4.0
petgraph            type param defaults                       gluon_parser-0.4.0
petgraph            type param defaults                       gluon_repl-0.4.0
petgraph            type param defaults                       kernel-1.1.0
petgraph            type param defaults                       parser-haskell-0.1.0
petgraph            type param defaults                       rose_tree-0.1.2
petgraph            type param defaults                       sqlpop-0.3.0
petgraph            type param defaults                       xswag-syntax-java-0.3.0
piston2d-graphics   type param defaults                       elmesque-0.12.0
syntex_syntax       illegal_struct_or_enum_constant_pattern   ctest-0.1.1
syntex_syntax       illegal_struct_or_enum_constant_pattern   rusty-cheddar-0.3.3
typemap             type param defaults                       magnet_core-0.0.1
typemap             type param defaults                       pencil-0.3.0
typemap             type param defaults                       plugin-0.2.6
typemap             type param defaults                       sharp_pencil-0.4.3

Legitimate failures:

False positives:

@petrochenkov
Copy link
Contributor Author

This is interesting, all the affected crates (except for abandoned locale) were fixed and published, but other crates still depend on older versions.
What would the best course of actions be?
Bump petgraph/typemap versions in dependent crates? Keep invalid_type_param_default a lint?

@nikomatsakis
Copy link
Contributor

@petrochenkov

This is interesting, all the affected crates (except for abandoned locale) were fixed and published, but other crates still depend on older versions.

Yeah, this is a common scenario. Frustrating. I'm not sure what's the best fix. Another option is to encourage the crate authors to push a new minor version that fixes the lint; we have done that in the past for major cases.

@bors
Copy link
Contributor

bors commented May 26, 2017

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

@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler @alexcrichton @brson -- so what should we do here? There are a number of outdated dependencies in crater which means that if we "close the door" on these compatibility lints, they will break.

One option:

  • Ping the authors here and encourage them to run cargo update and issue a new version, but land this code.
  • Ping the authors here, wait for (most of) them to issue new versions, then land the code.
  • Try to land PRs?

Seems like pinging authors is a good first step!

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 26, 2017

Ping the authors here and encourage them to run cargo update and issue a new version, but land this code.
Ping the authors here, wait for (most of) them to issue new versions, then land the code.
Try to land PRs?

I've already submitted PRs to all affected crates except for those depending on typemap and petgraph.
Next I was going to turn invalid_type_param_default into a lint again (leave it for later), and then propose to land this PR without waiting.

@alexcrichton
Copy link
Member

Thanks for the legwork here sending PRs @petrochenkov! It sounds like the major sources of breakage are or are about to all be mitigated? In that sense maybe leave invalid_type_param_default as a lint (warn for now to keep typemap/petgraph working) and then rerun crater?

@petrochenkov
Copy link
Contributor Author

Next I was going to turn invalid_type_param_default into a lint again (leave it for later), and then propose to land this PR without waiting.

Done.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 28, 2017

@petrochenkov @nikomatsakis Does this need another crater run? The status tag wasn't removed after the last one and I'm not clear on the current status based on discussion.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 28, 2017

@Mark-Simulacrum
I'd say no. It'll take time (weeks at least, from previous experience) for people to react on PRs I sent, so it shouldn't block landing this.

@Mark-Simulacrum
Copy link
Member

I'm a little confused by that; if we land this now, before the PRs are merged and published, presumably that would still break the ecosystem? Or am I missing something?

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 28, 2017

@Mark-Simulacrum
"break the ecosystem" is kinda overestimation, from non-updated broken crates only rusty-cheddar has dependent crates.
I'm personally okay with this kind of breakage, crates that are important in the ecosystem are usually fixed long before this stage + the breakage is nightly only, there are some weeks ahead to push the authors if someone needs their crates.

@Mark-Simulacrum
Copy link
Member

Ah, okay. Wasn't sure about the actual impact of these changes.

@bors
Copy link
Contributor

bors commented May 29, 2017

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

@nikomatsakis nikomatsakis removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 30, 2017
@nikomatsakis
Copy link
Contributor

r=me once rebased

@alexcrichton
Copy link
Member

@petrochenkov for future tracking purposes if you've still got links to the PRs to fix upstream crates, mind throwing them into the PR description? That way if we see regressions crop up on crater we can quickly find the upstream PR to fix it.

@petrochenkov
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 30, 2017

📌 Commit 26d5c0e has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 1, 2017

⌛ Testing commit 26d5c0e with merge 67aa270...

bors added a commit that referenced this pull request Jun 1, 2017
Turn sufficiently old compatibility lints into hard errors

It's been almost 7 months since #36894 was merged, so it's time to take the next step.

[breaking-change], needs crater run.

PRs/issues submitted to affected crates:
https://github.com/alexcrichton/ctest/pull/17
Sean1708/rusty-cheddar#55
m-r-r/helianto#3
azdle/virgil#1
rust-locale/rust-locale#24
mneumann/acyclic-network-rs#1
reem/rust-typemap#38

cc https://internals.rust-lang.org/t/moving-forward-on-forward-compatibility-lints/4204
cc #34537 #36887
Closes #36886
Closes #36888
Closes #36890
Closes #36891
Closes #36892
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jun 1, 2017

💔 Test failed - status-appveyor

@Mark-Simulacrum
Copy link
Member

Error looks potentially spurious, but hard to tell.

@bors retry

[01:25:51] failures:
[01:25:51]
[01:25:51] ---- cargo_compile_simple_git_dep stdout ----
[01:25:51]  running `C:\projects\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\cargo.exe build`
[01:25:51] running `C:\projects\rust\build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\cit\t3\foo\target\debug\foo.exe`
[01:25:51] thread 'cargo_compile_simple_git_dep' panicked at '
[01:25:51] Expected: execs
[01:25:51]     but: differences:
[01:25:51]   0 - |hello world|
[01:25:51]     +
[01:25:51]
[01:25:51]
[01:25:51] other output:
[01:25:51] ``', C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\hamcrest-0.1.1\src\core.rs:31
[01:25:51] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:25:51]
[01:25:51]
[01:25:51] failures:
[01:25:51]     cargo_compile_simple_git_dep
[01:25:51]
[01:25:51] test result: FAILED. 32 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 1, 2017
@alexcrichton
Copy link
Member

My guess is #33434

@brson
Copy link
Contributor

brson commented Jun 6, 2017

We're probably going to need writeups of all these for the release notes.

@petrochenkov petrochenkov deleted the oldhard branch August 26, 2017 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants