Skip to content

Add some guidance on rollups and priorities. - #402

Merged
XAMPPRocky merged 6 commits into
rust-lang:masterfrom
ehuss:rollup-priorities
Aug 21, 2020
Merged

XAMPPRocky merged 6 commits into
rust-lang:masterfrom
ehuss:rollup-priorities

Conversation

@ehuss

@ehuss ehuss commented Jul 17, 2020

Copy link
Copy Markdown
Contributor

This adds some guidance on setting rollup status and priority for the rust-lang/rust repo.

I waffled where to place this. I'm fine with moving it wherever. The rollups page was a high candidate, or a new page in the release or infra sections? No one group really "owns" the rust-lang/rust repo, so it is not really clear where it belongs.

I fully expect there to be some changes here, this is just a rough sketch that I wanted to start some discussion. This is based on some discussion at:

@ehuss

ehuss commented Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

cc @Manishearth

Comment thread src/compiler/reviews.md Outdated
- Your PR is not landing possibly-breaking or behavior altering changes.
- Feature stabilization without other changes is likely fine to
rollup, though.
- `rollup=maybe`: Use this if you don't have a high confidence that it won't

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note. this is the default if you have not mentioned antyhing

Comment thread src/compiler/reviews.md Outdated
specifically, as it would be hard to identify as the cause from a
rollup).
- Has a high chance of failure.
- A high-priority issue that needs to land ASAP.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should just be p=5 or something

Comment thread src/compiler/reviews.md
- Your PR is not landing possibly-breaking or behavior altering changes.
- Feature stabilization without other changes is likely fine to
rollup, though.
- `rollup=maybe`: This is the **default** if you do not specify a rollup

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

worth somehow clarifying that this is milder than iffy

Comment thread src/libs/maintaining-std.md Outdated
[RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html
[RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html
[Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops
[rollup guidelines]: ../compiler/reviews.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
[rollup guidelines]: ../compiler/reviews.md
[rollup guidelines]: ../compiler/reviews.md#rollup

Comment thread src/compiler/reviews.md Outdated
Comment on lines +43 to +46
- `rollup=maybe`: This is the **default** if you do not specify a rollup
status. Use this if you don't have much confidence that it won't break
tests. This can be used if you aren't sure if it should be one of the other
categories.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume rollupers likely add rollup=maybe PRs to rollup. So, I think it'd be nicer if we mentioned it and encouraged members to use iffy rather than maybe if there's any concerns?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, I don't really know. Like, I imagine there are quite a few PRs that do not fit the category of "iffy" (that is, "risky"), but the reviewer doesn't have enough confidence to mark it "always". Or, what is probably more common, is "I don't know what it should be", which I think is fine since I don't think we can expect everyone to be deeply familiar with the process and hazards of rollups.

@Manishearth any opinions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I basically think folks should never be explicitly marking things as rollup=maybe, it's the default, the reason you do it is to unmark something else from a diferent rollup level.

@ehuss

ehuss commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

Is there any other feedback anyone wants to give? Maybe @Mark-Simulacrum ?

@Mark-Simulacrum Mark-Simulacrum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks fine to me other than one nit.

It may also be worth noting that these docs should be somewhat less important with us moving to faster CI with GHA.

Comment thread src/compiler/reviews.md Outdated
implications. Example scenarios:
- Changes are limited to documentation, comments, etc. that is highly
unlikely to fail a build.
- Changes are pure refactoring and cannot have performance implications.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm I might remove the "pure refactoring" label here, it seems not obviously true that can't have performance implications.

@XAMPPRocky

Copy link
Copy Markdown
Contributor

@ehuss It seems this now has conflicts. Can you rebase this and then I'll merge it in?

@ehuss
ehuss force-pushed the rollup-priorities branch from ef18414 to ff27549 Compare August 20, 2020 20:10
@ehuss

ehuss commented Aug 20, 2020

Copy link
Copy Markdown
Contributor Author

Rebased, thanks!

@XAMPPRocky
XAMPPRocky merged commit 495a5b2 into rust-lang:master Aug 21, 2020
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.

5 participants