Regression report stable-2016-08-16 vs. beta-2016-08-26

[brson]

Root regressions, sorted by rank:

• encoding-0.2.32 (before) (after)
• rotor-0.6.3 (before) (after)
• ioctl-0.3.3 (before) (after)
• futures-0.1.0 (before) (after)
• genmesh-0.4.0 (before) (after)
• procinfo-0.2.3 (before) (after)
• daggy-0.4.0 (before) (after)
• nanomsg-sys-0.6.0 (before) (after)
• gluon-0.1.3 (before) (after)
• zmtp-0.6.0 (before) (after)
• locale-0.2.1 (before) (after)
• marksman_escape-0.1.2 (before) (after)
• colerr-1.0.0 (before) (after)
• iota-editor-0.1.0 (before) (after)
• json_rpc-0.2.0 (before) (after)
• couchdb-0.5.1 (before) (after)
• libxdo-0.4.0 (before) (after)
• cuticula-0.2.0 (before) (after)
• boiler-0.1.1 (before) (after)
• peano-1.0.1 (before) (after)
• plumbum-0.0.8 (before) (after)
• pmem-0.0.2 (before) (after)
• postgis-0.2.1 (before) (after)
• probor-0.2.0 (before) (after)
• endianrw-0.2.1 (before) (after)
• rodio-0.3.2 (before) (after)
• rose_tree-0.1.1 (before) (after)
• rustpather-0.1.0 (before) (after)
• rust-s3-0.2.3 (before) (after)
• fbx_direct-0.6.1 (before) (after)
• shuffled-iter-0.1.0 (before) (after)
• flexmesh-0.1.1 (before) (after)
• skeletal_animation-0.8.0 (before) (after)
• stdx-0.0.1 (before) (after)
• tick-0.0.1 (before) (after)
• glitter-0.1.0 (before) (after)

Full report.

[eddyb]

Breakdown:

• rustc stack overflows (#35408): boiler, marksman_escape and procinfo
• syntax error: rust-s3
• multiple dylib candidates for std found: nanomsg-sys
• String From / Borrow inference errors: gluon and zmtp
• unconstrained lifetime parameter: colerr (using an older owning_ref)
• deny(private_in_public): remaining 28 crates

[nikomatsakis]

deny(private_in_public): remaining 28 crates

Hmm. That’s pretty large. I have to say that I don’t feel great about this. Particularly since that rule is hardly a popular one – nor the most crucial. Perhaps it was too ambitious to imagine changing it and instead it should remain a lint forever (it is, in its own way, a kind of lint anyhow). (cc @aturon)

I think we need to drill more into the process for these warning -> error transitions. For example, here, because this warning was so old, I don’t see that we did any crater runs of impact (can that be true? I feel like I do remember some investigation?). However, for https://github.com/rust-lang/rust/pull/34982, we did a crater run, and then I made a point of ensuring that every affected crate was at minimum notified and, if possible, had a pending PR fixing the problem. I believe that for https://github.com/rust-lang/rust/pull/34923 we were similarly diligent.

[mitsuhiko]

Hmm. That’s pretty large. I have to say that I don’t feel great about this. Particularly since that rule is hardly a popular one – nor the most crucial.

It’s already a pretty big warning anyways. I actually find it quite useful during development that it’s just a warning and not an error. It tends to happen every once in a while and the fact that it does not immediately make the build fail I consider a nice feature.

[brson]

I also think it makes sense to revert this and rethink how to go forward on it. There’s no need to be hasty here.

[brson]

Thanks for digging through everything @eddyb.

[petrochenkov]

deny(private_in_public): remaining 28 crates

Abandoned projects maybe? private_in_public was reported as a warning since last autumn, i have hard time imagining how a project maintainer can compile his project and look at these warnings for so long and never silence them with #[allow(private_in_public)].[quote=“mitsuhiko, post:4, topic:3930”] I actually find it quite useful during development that it’s just a warning and not an error. [/quote] It’s still a warning, and it seems like it will proceed being a warning for a long time, it’s just deny-by-default now, no downstream projects affected and all that.

[petrochenkov]

Analysis of private_in_public errors:

• 5 crates - private traits in bounds / where clauses - these are supposed to go away if RFC 1671 is accepted.
• 10 crates - public reexports of private extern crates - this was fixed more recently (February 2016).
• 12 crates - other legit private-in-public errors reported since December 2015.

[lifthrasiir]

private_in_public was reported as a warning since last autumn, i have hard time imagining how a project maintainer can compile his project and look at these warnings for so long and never silence them with #[allow(private_in_public)].

As the first crate in the listing (ugh, sorry!), Encoding 0.3 was in the development since early this year (I think) and I had a fix to the warning in the issue list but didn’t realize that the 0.2 branch would have the same problem. This is also partly because I haven’t set the warning to fail the CI test—but if I had a separate branch for 0.2 I wouldn’t be able to see that as new Rust releases do not trigger CI anyway. (So, essentially, same to a pre-1.0 situation.)

I’ve taken this incident as an opportunity to actually apply the fix to 0.3 branch and also published a quick fix as 0.2.33. Still have to think about the long-term maintenance though…

[nikomatsakis]

@petrochenkov we talked about this in the core-team meeting. I think the conclusion was that we don’t feel comfortable with rolling out the private_in_public-deny change this way. We’d prefer to revert that change for the beta (since it’s an easy thing to do) and then we can have a separate discussion (i.e., now…) about precisely how we roll it out (do we need to notify crate authors? submit patches? should we just give up on making this a hard error altogether?)

Would you be up for making that PR?

One thing I don’t know is whether we think we should roll it back just on beta, or also on nightly (@brson, @alexcrichton, your thoughts on that?)

[brson]

I’d say on both nightly and beta. Reset things to the way they were just in case. But I think with the right preparation it’s reasonable to expect we’ll be confident to deploy to stable nov 10.

[alexcrichton]

Yeah I agree we should submit to nightly and we can then cherry-pick that to beta to fix both branches.

[petrochenkov]

@petrochenkov we talked about this in the core-team meeting. I think the conclusion was that we don’t feel comfortable with rolling out the private_in_public-deny change this way. We’d prefer to revert that change for the beta (since it’s an easy thing to do) and then we can have a separate discussion (i.e., now…) about precisely how we roll it out (do we need to notify crate authors? submit patches? should we just give up on making this a hard error altogether?)

Would you be up for making that PR?

OK. denying by default can be discussed again after deciding on RFC 1671. I’ll probably start sending PRs to affected crates in the meantime.

[nikomatsakis]

Thanks. Sorry for the confusion, still feeling our way on this sort of thing!

[petrochenkov]

The situation when a crate is fixed on GitHub, but the update is not published to crates.io turns out to be pretty common - about half of private-in-public regressions in the list. @brson in particular fixed stdx but never published stdx-0.0.2

[brson]

Hah. Well, stdx is completely useless, but I’ll put it on my list of projects for which I need to publish new builds.

[jseyfried]

The syntax error in rust-s3 was caused by #34600. We would have to partially unfix #34543 to make this error into a warning – c.f. this comment.

[jseyfried]

rust-s3 has been updated to work with beta/nightly.

[brson]

Thanks for the investigation @jseyfried.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.