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

major change proposal process for compiler team #2904

Merged
merged 8 commits into from
May 7, 2020

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 9, 2020

Introduce the major change process for the compiler team. This process has the following goals:

  • to advertise major changes and give the team a chance to weigh in;
  • to help scale the amount of work being done to reviewing bandwidth by choosing a reviewer in advance;
  • to avoid a lot of process overhead.

The intent here is that if you have a plan to make some "major change" to the compiler, you will start with this process. It may either simply be approved, but if the change proves more controversial or complex, we may escalate towards design meetings, longer write-ups, or full RFCs before reaching a final decision.

This process does not apply to adding new language features, but it can be used for minor features such as adding new -C flags to the compiler.

Rendered

@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the RFC. label Apr 9, 2020
@nikomatsakis
Copy link
Contributor Author

cc @rust-lang/compiler and @rust-lang/wg-governance

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

This seems like a good idea, and it seems like it further empowers non-team reviewers (compiler contributors) to land things without needing to check with the team by defining boundaries for when that's possible.

nikomatsakis and others added 2 commits April 9, 2020 14:07
Co-Authored-By: XAMPPRocky <4464295+XAMPPRocky@users.noreply.github.com>
@nikomatsakis
Copy link
Contributor Author

@rfcbot fcp merge

This has been discussed fairly extensively prior to opening the RFC (including a design meeting). Therefore, I'm going to move to FCP even though the RFC has only been open for 18 hours.

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 10, 2020

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Apr 10, 2020
@XAMPPRocky
Copy link
Member

One thing I would like to see added is depending on how frequent the major change process is used, is someone volunteering to post a short summary of the major changes that have happened on the “Inside Rust” blog every X (maybe 2–4) weeks to provide more visibility into what’s being worked on.

@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

@XAMPPRocky I feel that depends on the "majorness" of the major change; if it isn't too major, posting on "Inside Rust" feels like it would turn the lightweight process into something less lightweight.

@XAMPPRocky
Copy link
Member

@Centril Maybe to clarify I wasn't implying a post for a single change, I was thinking a format more like RELEASES.md where it's just a one or two sentences and a link to the PR for each of the changes in the past X weeks.

@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

@XAMPPRocky Oh; that is much less than I had imagined; seems reasonable. :)

@nikomatsakis

This comment has been minimized.

@Centril

This comment has been minimized.

@petrochenkov
Copy link
Contributor

I don't like that the process for stabilizing a minor user facing feature is different now between language/library and compiler.

Submitting a PR and starting an FCP worked well enough in all these cases.
(Except that waiting for one more week after everyone ticked their boxes often feels excessive.)

@Centril
Copy link
Contributor

Centril commented Apr 13, 2020

(Except that waiting for one more week after everyone ticked their boxes often feels excessive.)

I think the idea with that is that sometimes, many team members check their boxes very quickly (same day, e.g., in the case of the libs team should all of them) so it goes into FCP immediately, and then the community is given no time to object, if there is no waiting period.

@petrochenkov
Copy link
Contributor

many team members check their boxes very quickly (same day ... so it goes into FCP immediately

That never happens in practice though.

@Centril
Copy link
Contributor

Centril commented Apr 13, 2020

Well it does for the libs team but probably very rarely for the compiler team. :)

@nikomatsakis
Copy link
Contributor Author

@petrochenkov

I don't like that the process for stabilizing a minor user facing feature is different now between language/library and compiler.

Are you referring primarily to command line flags? I wasn't sure how to categorize them. I'd be happy to say that command-line flags and other "user-facing features" require full FCP.

@nikomatsakis
Copy link
Contributor Author

That said, I think we've had some confusion about what it takes to e.g. add a new -C flag. One of the things I was wanting to do was to follow up this RFC with a kind of "So you want to do X?" flow-chart, showing people how to make various sorts of changes and what kind of things are required.

For -C flags etc, I think the main thing that seems to often be missing is documentation as to the purpose of the flag etc (both end-user facing but also to help make an informed decision). This is precisely the sort of thing I would think the MCP would encourage, since it asks for more of a write-up than one often finds on a PR.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 13, 2020

So @Mark-Simulacrum and I were talking on Zulip and we wanted to tweak the process as follows:

  • You can "second" at any time by writing @rustbot second. This will trigger the final-comment-period label to be added and FCP lasts for 10 days. (There is no "complete checklist" in this case.)
  • We will announce things in FCP at the weekly triage meeting.
  • As long as the change is "reversible", and your confident all stakeholders will agree, you can shortcut the FCP, but it should still overlap with at least one triage meeting.

I'm mildly confused about the terminology of final-comment-period being confusing here, but I guess we can tweak that later.

@pnkfelix
Copy link
Member

I am fine with the proposal and protocol as described here.

I do think it would be good practice for us to explicitly link to the Zulip discussion, both on the Zulip server itself (so that people with Zulip accounts can readily jump in and join the discussion) and also on the zulip-archive, which serves as a publicly accessible log of the conversation (modulo edits to posted messages, since such edits are not currently included in the archive in any fashion).

@nikomatsakis
Copy link
Contributor Author

If we have a bot, it can link automatically the two, which would be nice.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Apr 19, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 19, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

text/0000-compiler-major-change-process.md Outdated Show resolved Hide resolved
text/0000-compiler-major-change-process.md Outdated Show resolved Hide resolved
@richkadel
Copy link

Additional observations... Can these be clarified, preferably in the template's checklist?

  1. There's an implication that the checkbox steps are supposed to be completed in order, but given my recent experience that's not entirely the case:

    Apparently, adding the proposal to the triage meeting agenda does not require checking off that a "second" has been found. (I don't have a "second" from the compiler team yet, but my MCP is "to-announce".) Is a "reviewer" even required before "to-announce"?

    If neither are required, I think it would be less confusing to reorder the check boxes and move the "announce" step above "find a reviewer" and "find a second".

  2. Since the "to-announce" label is automated or initiated by a team member, and not the author (per @Mark-Simulacrum 's comment above), what does the bot or team member use as criteria to know that "All edits are done" (required before entering this stage)? Can this criteria be explained? It seems like the author is the only one that would know if all edits are done, but there should be instructions (another checkbox preceding "announce", or a required comment by the author with a mention to a public/visible group?) for the author to acknowledge that edits are done and the MCP is ready to be announced for 1-week review.

  3. It's not clear how to identify a "reviewer" or a "second", especially for someone outside the compiler team. Is the compiler team willing to recommend or request volunteers for either the reviewer or at least for a "second" at the triage meeting?

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Apr 29, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 29, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 29, 2020
@nikomatsakis
Copy link
Contributor Author

@richkadel thanks for the concrete suggestions. I've got a few edits to make here...

@nikomatsakis
Copy link
Contributor Author

OK I think I included the edits. The biggest change is that I said that outward facing changes still require a rfcbot fcp at some point. I think that's fine, though I think we should do some follow-up work to document/streamline the procedure and expectations there for different sorts of outward facing changes.

@nikomatsakis nikomatsakis merged commit 87ca4f0 into rust-lang:master May 7, 2020
@nikomatsakis
Copy link
Contributor Author

Merged! thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.