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

CONTRIBUTING.md shouldn't suggest submodules pinned to PRs #44597

Closed
alexcrichton opened this issue Sep 15, 2017 · 10 comments
Closed

CONTRIBUTING.md shouldn't suggest submodules pinned to PRs #44597

alexcrichton opened this issue Sep 15, 2017 · 10 comments
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Right now the documentation suggests updating a submodule to the commit in a PR, but we want the submodules in the rust-lang/rust repository to be as long-lived as possible, in theory allowing you to check out rust-lang/rust at any point in time and run the build. PRs, however, will go away!

@alexcrichton
Copy link
Member Author

cc @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Sep 15, 2017

While PRs go away, when they are merged, their commits stay.

There's essentially no difference to creating a branch in each repo, other than that a PR could be closed or rebased. The same is true for branches, but less common.

We could require a locked "upstream" branch in each tool's repository, but that would not really work in the presence of multiple PRs that change the submodule. These PRs would have to be ordered then. I don't think that's realistic. We will merge any clippy PRs the moment the rust PR is merged to ensure the commits never get lost, but I don't see what else we can do.

@alexcrichton
Copy link
Member Author

That's not true if a PR is rebased, which happens very frequently. And yes up to now we've just created long-lived branches in upstream repos, for example see the LLVM repo.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 15, 2017

Github will deny loading a commit ID which isn't in a current PR. The moment you rebase in clippy, Rust's Travis will fail to load the old commit. So the rebase would have to happen after Rust's CI passes, but before we merge in clippy.

If this is still too volatile, I'll figure out how to create branches easily. Maybe we can automatically create branches from PRs with some keyword in their description. I want to reduce the interactions required by tool maintainers to a minimum, so rust contributors don't have to wait for timezones or anything

@alexcrichton
Copy link
Member Author

So the rebase would have to happen after Rust's CI passes, but before we merge in clippy.

That's true, yes, but I think this is still too fragile because often development can happen on the master branch while a PR is waiting to land, and that may mean by the time the Rust PR is merged the clippy PR needs a rebase to land on clippy master.

I want to reduce the interactions required by tool maintainers to a minimum, so rust contributors don't have to wait for timezones or anything

My thinking is that the "easy path" here is to just disable submodules building (via src/tools/toolstate.toml you're adding) and then later re-enable it. That way the submodule is basically always pointing at the master branch.

@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Sep 18, 2017
@steveklabnik
Copy link
Member

Triage: so, what's the conclusion here? Happy to document whatever is decided.

@steveklabnik
Copy link
Member

Traige: p-low, since i still don't have an answer ;)

@steveklabnik steveklabnik added the P-low Low priority label Nov 21, 2017
@steveklabnik
Copy link
Member

Triage: still waiting on what's to be decided, tagging in @rust-lang/compiler

@steveklabnik steveklabnik added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed P-low Low priority A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels May 29, 2018
@oli-obk
Copy link
Contributor

oli-obk commented May 29, 2018

by the time the Rust PR is merged the clippy PR needs a rebase to land on clippy master.

That's not a problem, We allow merging in the clippy repo. So instead of rebasing the PR you'd just merge master into it.

@steveklabnik I don't think there was any concious decision. But a system evolved:

  1. break the tool (automatic, you don't need to do anything)
  2. fix the tool (optional, but awesome!) and open a PR with the fix
  3. breaking PR is merged
  4. tool authors get pinged
  5. tool authors merge fix PR or fix themselves and update the submodule in rustc (e.g. to a rustc-staging locked branch on the submodule's repo)
  6. PR gets r+ed quicky and p=1

@steveklabnik
Copy link
Member

Great! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants