Stream: t-compiler

Topic: toolstate breakage


oli (Feb 27 2020 at 16:03, on Zulip):

Hi @T-compiler @RalfJ (pings for clippy ppl will be done on discord)
The status quo with our tool breakages works, but is not ideal imo. There are multiple forces at play here

  1. easy contributing to tools requires tools to be in their own repo (so we can't just make it all a monorepo)
  2. easy tool updates require tools to be in tree (so monorepo), because otherwise you need to fix the tool in its repo and then come back and update the submodule

Half a year ago I did a small investigation into different ways to handle projects with git. I found

  1. git submodule (what we use in Rust, but I've noticed elsewhere that ppl are quite confused/annoyed by submodules)
  2. git subtree (oh god experimenting with those was such a nonpleasure)
  3. git subrepo

Since I'm very much against git subtree, let me explain git subrepo.
TLDR: Monorepo with tooling to sync subfolders with other repos
Long version: git subrepo clone foo bar creates a folder bar with the contents of the repo foo. Now if contributors change rustc and a submodule, they can just modify the files directly (and we can gate all PRs on tools). The contributor really sees a monorepo and does not even need to know that the subrepo is a subrepo, they need not no learn any new commands beyond the standard git commands they need for any contribution.
The folder also gets a .subrepo file that has some metadata explaining how to sync said folder with the source repo. Now, this synchronization (I'll get to its details later) requires users to install git-subrepo and take care around rebasing because that messes up the .subrepo info, but this only applies to users that synchronize the subrepo. If you are just contributing to a tool via its own repo, you don't need to know about this. If you are just contributing to rustc and your changes requrie tool changes, you don't need to know about this.

Of course this is not free. We will end up with a snapshot of the tool in our tree (not the whole history thank the :spaghetti: :monster:). But other than that, it's totally cheap to do and we can do this change per tool without any hurry.

nagisa (Feb 27 2020 at 16:05, on Zulip):

-1 on having to install additional tooling, but from your description it is not entirely clear who are the users that synchronize the subrepo

oli (Feb 27 2020 at 16:06, on Zulip):

clippy devs would do the syncing just like they do now

oli (Feb 27 2020 at 16:06, on Zulip):

noone needs to sync who doesn't want to sync

oli (Feb 27 2020 at 16:06, on Zulip):

the extra tooling is only required for ppl with an interest in syncing

oli (Feb 27 2020 at 16:07, on Zulip):

basically in the clippy case we sync when we want to use a newer nightly on the clippy repo or when we want to move new features from the clippy repo to the rustc subrepo

pnkfelix (Feb 27 2020 at 16:07, on Zulip):

I'm trying to understand how someone making rustc changes that have effects on tools wouldn't need to know about.subrepo

oli (Feb 27 2020 at 16:07, on Zulip):

there's a literal cp copy of the working copy of the subrepo in the rustc working copy

nagisa (Feb 27 2020 at 16:08, on Zulip):

Also this sounds like divergence between the external repo and the rustc repo is possible? How is that handled?

pnkfelix (Feb 27 2020 at 16:08, on Zulip):

the changes to the tool just turn into commits on the rust-lang/rust repo ?

centril (Feb 27 2020 at 16:08, on Zulip):

@oli can you elaborate on the rebasing aspect; when I'm updating my PR that isn't making tool changes, can I continue just using git pull --rebase upstream master?

oli (Feb 27 2020 at 16:08, on Zulip):

you can make commits that cross the rustc repo and the external repo

oli (Feb 27 2020 at 16:08, on Zulip):

pnkfelix said:

the changes to the tool just turn into commits on the rust-lang/rust repo ?

yes

pnkfelix (Feb 27 2020 at 16:08, on Zulip):

and the synchronizers have to deal with cherry-picking them?

oli (Feb 27 2020 at 16:08, on Zulip):

ah no

oli (Feb 27 2020 at 16:08, on Zulip):

you only get one commit per sync

pnkfelix (Feb 27 2020 at 16:08, on Zulip):

oh

oli (Feb 27 2020 at 16:08, on Zulip):

just like with submodule

oli (Feb 27 2020 at 16:08, on Zulip):

but the clippy repo gets all commits that touched its subrepo in the rustc repo

oli (Feb 27 2020 at 16:09, on Zulip):

centril said:

oli can you elaborate on the rebasing aspect; when I'm updating my PR that isn't making tool changes, can I continue just using git pull --rebase upstream master?

yes

pnkfelix (Feb 27 2020 at 16:09, on Zulip):

so in the rust-lang/rust --> tool repo direction, its fine grained?

oli (Feb 27 2020 at 16:09, on Zulip):

only if you used a git subrepo command do you need to take care

oli (Feb 27 2020 at 16:09, on Zulip):

pnkfelix said:

so in the rust-lang/rust --> tool repo direction, its fine grained?

yes

pnkfelix (Feb 27 2020 at 16:09, on Zulip):

but int he tool repo --> rust-lang/rust direction, its coarse grained?

oli (Feb 27 2020 at 16:09, on Zulip):

yes, it's one commit per update in the tool repo -> rust repo direction

pnkfelix (Feb 27 2020 at 16:10, on Zulip):

that probably makes sense.

nagisa (Feb 27 2020 at 16:10, on Zulip):

So basically the benefit here is that this is just not submodule?

oli (Feb 27 2020 at 16:10, on Zulip):

yes

oli (Feb 27 2020 at 16:10, on Zulip):

it's a monorepo for all intents and purposes

oli (Feb 27 2020 at 16:10, on Zulip):

except with synchronization tooling so we can keep a separate repo for easy contribs by users who contrib to the tools

centril (Feb 27 2020 at 16:11, on Zulip):

If I am hacking on rustc and also need to make drive-by changes to Clippy and then there's a merge conflict in the clippy changes, what happens then?

oli (Feb 27 2020 at 16:11, on Zulip):

the synchronizer needs to resolve them

oli (Feb 27 2020 at 16:12, on Zulip):

your changes are no different from a normal PR to clippy

nagisa (Feb 27 2020 at 16:12, on Zulip):

What (workflow?) improvements do we expect here? From my reading you basically just get submodules under a different name with slightly different and way more obscure semantics to be honest.

oli (Feb 27 2020 at 16:12, on Zulip):

we get to block CI on tool breakage

oli (Feb 27 2020 at 16:12, on Zulip):

so if you do some changes that break tools, you can fix them in your PR

oli (Feb 27 2020 at 16:12, on Zulip):

without a sync dance

oli (Feb 27 2020 at 16:12, on Zulip):

you just fix them inside the rust repo

centril (Feb 27 2020 at 16:13, on Zulip):

:+1:

nagisa (Feb 27 2020 at 16:13, on Zulip):

Again, does that mean the tool repo and the rustc repo can diverge for that tool?

centril (Feb 27 2020 at 16:13, on Zulip):

imo this almost sounds too good to be true :P

oli (Feb 27 2020 at 16:13, on Zulip):

nagisa said:

Again, does that mean the tool repo and the rustc repo can diverge for that tool?

yes

nagisa (Feb 27 2020 at 16:14, on Zulip):

And how are we gonna handle non-trivial… lets call them… sync conflicts?

oli (Feb 27 2020 at 16:15, on Zulip):

just like we handle them right now, they can already happen after contribs to the clippy repo become conflicting with future changes required for rustups

Jack Huey (Feb 27 2020 at 16:15, on Zulip):

Could there be automatic syncs through bors or something?

nagisa (Feb 27 2020 at 16:15, on Zulip):

With submodules you won’t get any conflicts inside a tool though.

nagisa (Feb 27 2020 at 16:16, on Zulip):

only between separate PRs against that tool.

nagisa (Feb 27 2020 at 16:16, on Zulip):

now you get yet another family of conflicts.

nagisa (Feb 27 2020 at 16:18, on Zulip):

And it requires a 3rd party, that’s potentially not familiar with changes in question, to resolve them.

pnkfelix (Feb 27 2020 at 16:18, on Zulip):

nagisa said:

With submodules you won’t get any conflicts inside a tool though.

you just get periods of time where the nightly rust doesn't have a working tool

pnkfelix (Feb 27 2020 at 16:18, on Zulip):

(but honestly, maybe that should be considered "okay")

nagisa (Feb 27 2020 at 16:19, on Zulip):

pnkfelix said:

you just get periods of time where the nightly rust doesn't have a working tool

I think that’s the choice we made entirely for convenience reasons though? There’s no inherent technical limitation that would prevent us from CIing tools in rustc and requiring rustc PR authors to also fix tools at the same time.

nagisa (Feb 27 2020 at 16:20, on Zulip):

this proposal is effectively that and the only thing it wins is not having to go to another repo to fix it (so that the changes to tools and changes to rustc can land in synchronized manner)

oli (Feb 27 2020 at 16:21, on Zulip):

yes, but these "only wins" are really big blockers and I believe the explicit syncs by ppl will be less of a problem because they require you to just handle conflicts with other PRs to the same tool, not between different repos

nagisa (Feb 27 2020 at 16:22, on Zulip):

I didn’t mean to imply that the wins are irrelevant.

nagisa (Feb 27 2020 at 16:23, on Zulip):

Either way, my opinion is that this trades one kind of clunky with another kind of clunky. I guess the benefits of the other kind of clunky are sizable. But IME ideally we get both the benefits and no clunky.

simulacrum (Feb 27 2020 at 16:25, on Zulip):

I think it's also true that this in theory makes it super easy to move to monorepo if we ever want to, right?

simulacrum (Feb 27 2020 at 16:25, on Zulip):

i.e. you just stop syncing?

nagisa (Feb 27 2020 at 16:26, on Zulip):

One alternative option I can think is moving the source of truth to an actual rustc monorepo and then clippy devs just sync the portion of the tree they are interested in into another repo once in a while. Mainlining the changes from their own repo would then be a regular PR against rustc.

simulacrum (Feb 27 2020 at 16:26, on Zulip):

That's the subrepo thing basically, right?

simulacrum (Feb 27 2020 at 16:27, on Zulip):

Or at least I'm not really seeing a difference

nagisa (Feb 27 2020 at 16:28, on Zulip):

In the other direction, if I understand the subrepo correctly. But then any concerns arising from clippy devs’ workflows are clippy devs’ responsibility to deal with at the time they mainline changes from their repo into rustc.

nagisa (Feb 27 2020 at 16:28, on Zulip):

and it does not require any additional tooling on anybody’s end.

simulacrum (Feb 27 2020 at 16:31, on Zulip):

I think that where the canonical repository is doesn't matter, right?

simulacrum (Feb 27 2020 at 16:31, on Zulip):

Like, AFAICT, for all intents and purposes, rustc is the canonical repository

simulacrum (Feb 27 2020 at 16:32, on Zulip):

It's just that much of the work happens elsewhere and is periodically rolled in

centril (Feb 27 2020 at 16:33, on Zulip):

Definitely agree with @oli that the wins here are pretty big

centril (Feb 27 2020 at 16:33, on Zulip):

having to jump between repos when hacking on rustc would imo be a non-starter

nagisa (Feb 27 2020 at 16:34, on Zulip):

How hard is it to improve our rustc workflows to make actual genuine monorepo tenable?

nagisa (Feb 27 2020 at 16:35, on Zulip):

What are the current concerns?

centril (Feb 27 2020 at 16:36, on Zulip):

my understanding is that the concerns are re. compile times / landing times

centril (Feb 27 2020 at 16:36, on Zulip):

but Clippy doesn't have that many PRs per week

centril (Feb 27 2020 at 16:37, on Zulip):

I personally think it's tenable already; on the upside, people like myself would now be interested in improving clippy

centril (Feb 27 2020 at 16:37, on Zulip):

but moving to GHA should help to reduce landing times substantially

nagisa (Feb 27 2020 at 16:38, on Zulip):

compile times can remain mostly unaffected, you’d just do the usual cargo test from the clippy directory, no?

centril (Feb 27 2020 at 16:38, on Zulip):

I think so

nagisa (Feb 27 2020 at 16:39, on Zulip):

Running partial tests would help most with the landing times, but I doubt we want to go there.

centril (Feb 27 2020 at 16:39, on Zulip):

yea don't ever break the no-rocket science rule

centril (Feb 27 2020 at 16:40, on Zulip):

though ostensibly if you only change things in the clippy folder then you can avoid running every single builder

centril (Feb 27 2020 at 16:41, on Zulip):

but I suspect the infra team would be unhappy with that

simulacrum (Feb 27 2020 at 16:44, on Zulip):

We can't really do that today, too prone to breakage (and you want artifacts for every bors commit which means a full build)

simulacrum (Feb 27 2020 at 16:44, on Zulip):

I feel like we should do subrepo now and then maybe more than that later if we want to

simulacrum (Feb 27 2020 at 16:45, on Zulip):

I think subrepo gets us 90-95% of the way there though

simulacrum (Feb 27 2020 at 16:46, on Zulip):

@oli would you be on board to try and get that going? Maybe we can get a PR up making the changes necessary and FCP merge it or so?

centril (Feb 27 2020 at 16:49, on Zulip):

As a rollout plan, we could first try subrepo for maybe 2 months and then if all goes well we can start gating on e.g. Clippy

Philipp Krones (Feb 27 2020 at 20:13, on Zulip):

So if I want to update clippy, it would look like this:
Let's say Clippy was last synced at commit X. The Clippy repo got 20 commits since then -> X+20, while the rust-Clippy repo got 3 fixing commits -> X+3

I then run git subrepo sync (or whatever) and resolve all sync conflicts. Then the Clippy repo is at (X+20)+3 and the rust-Clippy repo at (X+3)+1, since all commits from clippy got squashed in a single sync commit.

I then open a PR in each repo and update the nightly version in the Clippy repo.

A few questions:
Did I get it right up until here?
What if there is a sync fallout? Do I add commits to both repos/PRs until they are fixed?

RalfJ (Feb 28 2020 at 09:16, on Zulip):

Hm, that's very interesting indeed. I guess whether or not this is better than toolstate breakage depends on how painful the syncing is.

RalfJ (Feb 28 2020 at 09:17, on Zulip):

to what extend could we have that automated? like, whenever a commit lands in rustc that changes the subrepo, have a bot open a syncing PR against clippy/miri? the more timely this is done, the lower the chance of conflicts.

lzutao (Feb 28 2020 at 14:36, on Zulip):

What about testing? As far as I know, clippy runs it integration tests on other crates to look for crashing and ICE.

Manish Goregaokar (Feb 28 2020 at 17:00, on Zulip):

@oli I think this is good, but it's only going to be an improvement if we also start gating on it in the compiler imo. Which shouldn't be a big deal for compiler folks, it's just another in-tree consumer like rustdoc and 99% of breakages are ones where the compiler author is fixing such things all over the codebase _anyway_. (and ideally we should suggest this approach to rustdoc as well)

Josh Triplett (Feb 28 2020 at 17:43, on Zulip):

I really don't like the idea of requiring people to have another git tool installed.

Josh Triplett (Feb 28 2020 at 17:44, on Zulip):

Git workflows involving third-party tools always look simple when you look at the simple commands, but completely break more advanced git usage.

Josh Triplett (Feb 28 2020 at 17:45, on Zulip):

I would love to figure out how we can make a monorepo viable in performance for tool developers.

Josh Triplett (Feb 28 2020 at 17:45, on Zulip):

That seems doable.

Josh Triplett (Feb 28 2020 at 17:46, on Zulip):

Failing that, submodule doesn't seem any worse (and we already use it), and it is at least built into git.

simulacrum (Feb 28 2020 at 17:57, on Zulip):

@Josh Triplett Well, to be clear, my understanding is that we don't require anyone except people doing the upstreaming work to install said tool

simulacrum (Feb 28 2020 at 17:58, on Zulip):

(and even then that tool is only "nice to have" -- you can always just cp or rsync the files or so between the two directories, AFAICT)

simulacrum (Feb 28 2020 at 17:58, on Zulip):

submodule is much worse -- it doesn't support bidirectional editing at all

Josh Triplett (Feb 28 2020 at 18:46, on Zulip):

Wouldn't anyone trying to do work in rustc and commit changes that include changes to tools needed it?

simulacrum (Feb 28 2020 at 18:47, on Zulip):

no, not as I understand it

simulacrum (Feb 28 2020 at 18:47, on Zulip):

just if you're syncing between the two repos

simulacrum (Feb 28 2020 at 18:47, on Zulip):

which while not infrequent, is done by very few peopel

simulacrum (Feb 28 2020 at 18:48, on Zulip):

to everyone else it's just a bunch of files in a subdirectory, nothing special

Josh Triplett (Feb 28 2020 at 18:50, on Zulip):

Ah, that wasn't clear from the discussion above.

Josh Triplett (Feb 28 2020 at 18:50, on Zulip):

If this doesn't change the workflow for anyone else working in rustc, then I withdraw my objection.

nikomatsakis (Mar 03 2020 at 20:31, on Zulip):

(catching up) this does seem pretty interesting. I think it's quite relevant also to efforts to "library-ify" the compiler.

Manish Goregaokar (Mar 29 2020 at 21:53, on Zulip):

@oli could you file an issue about this? perhaps we should start trying this out for Clippy

Manish Goregaokar (Mar 29 2020 at 21:53, on Zulip):

and maybe move rustfmt over so it doesn't need the rustc-ap stuff anymore

centril (Mar 30 2020 at 08:15, on Zulip):

Manish Goregaokar said:

and maybe move rustfmt over so it doesn't need the rustc-ap stuff anymore

That'd be great; I could then help out with some cleanups to the parser logic

matthiaskrgr (Mar 30 2020 at 13:55, on Zulip):

do I understand correctly that with the subrepo, we will have to fix actual merge conflicts in addition to 'only' compile errors if clippy breaks? (imo that sounds quite a bit more complicated...)

matthiaskrgr (Mar 30 2020 at 13:56, on Zulip):

or will that completely eliminate clippy breaking?

matthiaskrgr (Mar 30 2020 at 13:57, on Zulip):

(also our current CI setup is more strict than just ./x.py test src/tools/clippy, that might cause problems here and there)

simulacrum (Mar 30 2020 at 14:08, on Zulip):

I think yes, you'd need to rebase atop clippy changes in rust-lang/rust, i.e. merge conflicts, but we would presumably also make clippy never broken. It would basically mean that clippy is equivalent of src/tools/rustdoc, I think.

simulacrum (Mar 30 2020 at 14:09, on Zulip):

(Except has a separate repository for issues and PRs)

eddyb (Mar 30 2020 at 14:42, on Zulip):

I want this, so much

eddyb (Mar 30 2020 at 14:42, on Zulip):

I just want to be able to fix tools when I make a change in rustc and not leave it to other people, or navigate several repos

eddyb (Mar 30 2020 at 14:43, on Zulip):

it's entirely unnecessary to ever have "broken tools" as a possible state

eddyb (Mar 30 2020 at 14:43, on Zulip):

I think we used to do a juggle with submodules to try and never break something but it got really hard

eddyb (Mar 30 2020 at 14:43, on Zulip):

(we probably still have to do it for Cargo, right?)

eddyb (Mar 30 2020 at 14:44, on Zulip):

@matthiaskrgr we should also move all the strictest testing to be able to run from x.py, so that nothing can break

simulacrum (Mar 30 2020 at 14:45, on Zulip):

yeah, for cargo we do, but since cargo is very high level and usually only depends on stabilized features in a "breaking" way it's not too bad

simulacrum (Mar 30 2020 at 14:45, on Zulip):

i.e. cargo tries to not have tests for unstable features (in rustc)

matthiaskrgr (Mar 30 2020 at 14:55, on Zulip):

eddyb said:

matthiaskrgr we should also move all the strictest testing to be able to run from x.py, so that nothing can break

uuh..
well, what we do doing ci is stuff like 1) build clippy 2) test clippy 3) run doctests of clippy-driver 4) check(bootstrap) clippy with clippy to make sure there are no warnings (also internal warnings/lints) 5) check formatting with cargo fmt 6) check formatting of .mds 7) run clippy against ~15 rust projects and check if there are ICSs/panics

matthiaskrgr (Mar 30 2020 at 14:55, on Zulip):

all this would probably take around an hour or so if run locally :sweat_smile:

matthiaskrgr (Mar 30 2020 at 14:57, on Zulip):

./x.py test src/tools/clippy --hurt-me-plenty

eddyb (Mar 30 2020 at 15:01, on Zulip):

hmm

eddyb (Mar 30 2020 at 15:01, on Zulip):

ah but none of those should really block clippy from being present in nightly

matthiaskrgr (Mar 30 2020 at 15:15, on Zulip):

it will probably happen that someone changes clippy in the rustcrepo and when we try to sync these changes into the upstream repo, we notice the crash and need to wait a couple of days for an upstream (rustc) fix and can't land the sync because of that (or have to temporarily disable the ci check(s)...)

matthiaskrgr (Mar 30 2020 at 15:16, on Zulip):

meanwhile a crashing clippy gets shipped in nightly and everyone complains :P

matthiaskrgr (Mar 30 2020 at 15:16, on Zulip):

(although we already had similar situations with the current system, so whatever...)

centril (Mar 30 2020 at 15:17, on Zulip):

This seems like a testable hypothesis over some time before we always gate on clippy

eddyb (Mar 30 2020 at 15:19, on Zulip):

why would clippy crash?

eddyb (Mar 30 2020 at 15:19, on Zulip):

or do you mean on some specific project we don't test

eddyb (Mar 30 2020 at 15:20, on Zulip):

you could still run CI using nightly clippy, so we're informed immediatelly

matthiaskrgr (Mar 30 2020 at 15:22, on Zulip):

eddyb said:

why would clippy crash?

it is only written by mortal humans .. :P

well sometimes clippy code triggers rustc paths that are not commonly used and that triggers an ICE that is not reproducible by running plain rustc on the source code

eddyb (Mar 30 2020 at 15:23, on Zulip):

yes but I mean, we actually test tools

eddyb (Mar 30 2020 at 15:23, on Zulip):

in Rust CI

eddyb (Mar 30 2020 at 15:23, on Zulip):

and once we're no longer allowing build failures, there's pretty much 0 reason to allow test failures

matthiaskrgr (Mar 30 2020 at 15:24, on Zulip):

there have been panics caused by code that we did not have tests for previously, but that was caught by running clippy against cargo sources (+ all its deps) for example

eddyb (Mar 30 2020 at 15:25, on Zulip):

we'll hopefully be able to do more of that once we switch to GHA

matthiaskrgr (Mar 30 2020 at 15:25, on Zulip):

https://github.com/rust-lang/rust-clippy/blob/d8e6e4cfcd83d555bd7717ea24224b777ed75773/.github/workflows/clippy_bors.yml#L233

eddyb (Mar 30 2020 at 15:25, on Zulip):

cc @Pietro Albini

Pietro Albini (Mar 30 2020 at 15:26, on Zulip):

it's not like pings are speeding up the GHA development :stuck_out_tongue:

eddyb (Mar 30 2020 at 15:26, on Zulip):

I meant the part about potentially testing more in CI once it's faster :P

Manish Goregaokar (Mar 31 2020 at 16:31, on Zulip):

@matthiaskrgr we would never have to fix compile errors again, provided we gate on clippy building. we could also gate on clippy tests passing. We would just have to do periodic merges, which are way easier since they don't require context

Manish Goregaokar (Mar 31 2020 at 16:32, on Zulip):

@centril i think we should start by gating on it building (it's pretty meaningless if we're not doing that since people will just ignore it if CI isn't telling them not to), and then perhaps try gating on tests as well to see what happens

centril (Mar 31 2020 at 16:33, on Zulip):

@Manish Goregaokar that could work

Manish Goregaokar (Mar 31 2020 at 16:33, on Zulip):

we could potentially turn off some of the more annoying tests, like the dogfood test, and instead have _that_ get run on clippy's nightly build

Manish Goregaokar (Mar 31 2020 at 16:34, on Zulip):

alternatively we can run the dogfood test as an integration test without gating on the lint result

Manish Goregaokar (Mar 31 2020 at 16:34, on Zulip):

ideally, fixing clippy should be very easy for people breaking it, so we should avoid gating on fmt/dogfood

Manish Goregaokar (Mar 31 2020 at 16:34, on Zulip):

(fmt i can go either way)

eddyb (Mar 31 2020 at 16:35, on Zulip):

I can't emphasize enough how much I want to fix tools when I break them :D

oli (Mar 31 2020 at 16:40, on Zulip):

I'll start writing usage docs and do this for clippy, anyone volunteering for reviewing tomorrow morning with closing the tree and priorizing and stuff?

bjorn3 (Mar 31 2020 at 16:40, on Zulip):

(wrong thread)

eddyb (Mar 31 2020 at 16:41, on Zulip):

@oli I have it on good authority that @centril loves closing the tree :P

oli (Mar 31 2020 at 16:41, on Zulip):

hehe

centril (Mar 31 2020 at 16:41, on Zulip):

@oli we can coordinate, in #infra

centril (Mar 31 2020 at 16:41, on Zulip):

(on Discord)

oli (Mar 31 2020 at 16:44, on Zulip):

ok

RalfJ (Mar 31 2020 at 16:46, on Zulip):

eddyb said:

yes but I mean, we actually test tools

AFAIK, right now Miri is the only tool that is actually not shipped via rustup when tests fail. for most tools, if they build, we ship them. (the builders that create the package dont have time budget to run the tests, so they don't know better.)
however for some time now we have the infra to gate shipping tools on their tests, just no tool other than miri uses that.

RalfJ (Mar 31 2020 at 16:47, on Zulip):

@oli we talked about doing this for Miri but there was some problem with the "push" action also having a commit on the rustc side?

oli (Mar 31 2020 at 16:47, on Zulip):

yes

RalfJ (Mar 31 2020 at 16:48, on Zulip):

so is that a solved problem or what do we do?

RalfJ (Mar 31 2020 at 16:48, on Zulip):

(FWIW, if trying this with clippy first is considered too risky I am okay with using Miri as the guinea pig here.)

oli (Mar 31 2020 at 16:48, on Zulip):

It may be solvable by storing the synchronization file in the miri repo... I'll check if subrepo supports this

oli (Mar 31 2020 at 16:57, on Zulip):

doesn't look like it. A push will always need to store the parent of the push-commit's ID together with the remote repo's new commit id in a .subrepo file in order for the next pull or push to be able to figure out what changed and only push that

oli (Mar 31 2020 at 16:58, on Zulip):

it can't be in miri, as theoretically multiple repos could be using miri as a subrepo

oli (Mar 31 2020 at 17:02, on Zulip):

So when you push, you get a commit to rustc that you need to merge without ever rebasing it

oli (Mar 31 2020 at 17:03, on Zulip):

Hypothetically we could make bors do this after every merge that touched a subrepo

oli (Mar 31 2020 at 17:04, on Zulip):

or... PRs that only modify the .subrepo file are merged without running any CI

Manish Goregaokar (Mar 31 2020 at 17:11, on Zulip):

@oli down to help. we should have a github issue filed though

Manish Goregaokar (Mar 31 2020 at 17:11, on Zulip):

i think aftger clippy we should ask rustfmt to try movng over, and maybe rustdoc

eddyb (Mar 31 2020 at 17:45, on Zulip):

rustdoc?

eddyb (Mar 31 2020 at 17:45, on Zulip):

rustdoc is still in-tree :P

Manish Goregaokar (Mar 31 2020 at 17:45, on Zulip):

Right, it's worth talking to imperio and seeing if they'd prefer to move it out of tree via subrepo

Manish Goregaokar (Mar 31 2020 at 17:47, on Zulip):

miri and clippy are already submodules so this is a strict win and also an easy move to make. rustfmt has that -ap- stuff so it's going to be a bit more involved getting rid of it, but it would benefit too. rustdoc is in tree so it's a bunch more work to move out, but it might be beneficial

eddyb (Mar 31 2020 at 17:48, on Zulip):

ah I see, sure

eddyb (Mar 31 2020 at 17:48, on Zulip):

rls is the other one

eddyb (Mar 31 2020 at 17:48, on Zulip):

if I had the ability to touch rls in tree, maybe I'd start cleaning it up

eddyb (Mar 31 2020 at 17:48, on Zulip):

mmmaybe

eddyb (Mar 31 2020 at 17:49, on Zulip):

it would certainly be more tempting than otherwise :P

Philipp Hansch (Mar 31 2020 at 19:07, on Zulip):

this is the subrepo command we would be using right? https://github.com/ingydotnet/git-subrepo

Philipp Krones (Mar 31 2020 at 19:19, on Zulip):

i think we should start by gating on it building (it's pretty meaningless if we're not doing that since people will just ignore it if CI isn't telling them not to), and then perhaps try gating on tests as well to see what happens

IMO we should at least also gate on the Clippy UI-tests from the get go. If we don't do this and there's a regression from a in tree rustup, we would have to go through all the PRs that fixed something in Clippy to figure out, what caused this regression. This can be really hard, since finding what causes some test regressions is sometimes already hard if you know what rust PR broke the test.

I think we should start by gating on build and ui-test for the Clippy repo. Maybe add dogfood later (and hope that it doesn't lead to #[allow(clippy::_)] from every Clippy in tree fix). Integration tests, fmt, and remark can only be run in the Clippy repo, since they are rarely affected by a rustup.

matthiaskrgr (Mar 31 2020 at 23:15, on Zulip):

Hmm, another thing; right now, I can simply do x.py check and when that succeeds, I assume that my changes build.
Once we gate on clippy, will it always require a 2 stage build (which takes can take ~50 times longer than ./x.py check) to see if clippy still works?
Or is it possible to have some kind of shortcut?

centril (Mar 31 2020 at 23:20, on Zulip):

we'd need to make ./x.py check also check clippy

centril (Mar 31 2020 at 23:21, on Zulip):

like it does check rustdoc

matthiaskrgr (Mar 31 2020 at 23:26, on Zulip):

then we need at least a stage 1 for that I think

matthiaskrgr (Mar 31 2020 at 23:26, on Zulip):

since you can't build nightly clippy with beta

centril (Mar 31 2020 at 23:30, on Zulip):

what's significant difference between rustdoc and clippy here?

matthiaskrgr (Mar 31 2020 at 23:35, on Zulip):

you can cargo +beta check rustdoc but not clippy , at least I think so

eddyb (Apr 01 2020 at 00:43, on Zulip):

@matthiaskrgr heh, no

eddyb (Apr 01 2020 at 00:43, on Zulip):

rustdoc is 100% unstable rustc internals

eddyb (Apr 01 2020 at 00:44, on Zulip):

@centril is right about this, it's just a bit of missing support

eddyb (Apr 01 2020 at 00:45, on Zulip):

all rustdoc, clippy or miri need, in cargo check them, is the cargo check'd librustc_* libs

eddyb (Apr 01 2020 at 00:45, on Zulip):

so ./x.py check could totally work

eddyb (Apr 01 2020 at 00:45, on Zulip):

also ./x.py test --stage 1 src/test/rustdoc has worked for ages, and it compiles rustc once and rustdoc once

eddyb (Apr 01 2020 at 00:46, on Zulip):

we wanted to setup something like this for miri but miri does a few weird things we'll have to take into account

matthiaskrgr (Apr 01 2020 at 01:00, on Zulip):

oh great :+1:

eddyb (Apr 01 2020 at 12:14, on Zulip):

@oli hang on... this git subrepo thing.... is it just an external tool that makes commits?

oli (Apr 01 2020 at 12:15, on Zulip):

yes

eddyb (Apr 01 2020 at 12:15, on Zulip):

...

eddyb (Apr 01 2020 at 12:15, on Zulip):

okay I was expecting something internal to git, where it tracks trees separately like a submodule

eddyb (Apr 01 2020 at 12:15, on Zulip):

git subtree looks far more appropriate

oli (Apr 01 2020 at 12:15, on Zulip):

no, this is just fancy tooling around monorepos with synchronization helpers

eddyb (Apr 01 2020 at 12:15, on Zulip):

but we don't want a monorepo, git-wise, do we?

eddyb (Apr 01 2020 at 12:16, on Zulip):

wouldn't git subtree let you have the exact commit history that is now in submodules?

eddyb (Apr 01 2020 at 12:16, on Zulip):

or am I missing something?

oli (Apr 01 2020 at 12:17, on Zulip):

I just read in random places that ppl were unhappy with git subtree

oli (Apr 01 2020 at 12:17, on Zulip):

so I looked for alternatives

oli (Apr 01 2020 at 12:17, on Zulip):

I never actually used git subtree myself

eddyb (Apr 01 2020 at 12:17, on Zulip):

okay but git subrepo isn't a real git integration, which is what I (and probably others) were led to believe

oli (Apr 01 2020 at 12:19, on Zulip):

oh

oli (Apr 01 2020 at 12:19, on Zulip):

I specifically stated you need to install a separate tool

oli (Apr 01 2020 at 12:19, on Zulip):

and it's completely transparent otherwise

eddyb (Apr 01 2020 at 12:19, on Zulip):

yes but it doesn't interact with git repos and their commits in a nested way

eddyb (Apr 01 2020 at 12:19, on Zulip):

the name is sort of a lie

oli (Apr 01 2020 at 12:20, on Zulip):

ok, shrug

eddyb (Apr 01 2020 at 12:20, on Zulip):

I'll leave a comment on the issue

oli (Apr 01 2020 at 12:20, on Zulip):

someone else do sth else then :D I don't know git subtree

eddyb (Apr 01 2020 at 12:21, on Zulip):

I guess I can try to nuke the clippy submodule and add it as a subtree, like you did

eddyb (Apr 01 2020 at 12:21, on Zulip):

and see what happens

centril (Apr 01 2020 at 12:23, on Zulip):

@eddyb what's important here is that we can make changes directly via rust-lang/rustto clippy without having to make a separate PR in clippy

centril (Apr 01 2020 at 12:23, on Zulip):

so make sure subtree or whatever the alternative is handles that

eddyb (Apr 01 2020 at 12:24, on Zulip):

@centril I mean, yes, that's the bare absolute minimum, but also we should avoid degrading the quality of git history

eddyb (Apr 01 2020 at 12:24, on Zulip):

@oli oh, git subtree does the thing you did for merging miri into rustc lmao

eddyb (Apr 01 2020 at 12:25, on Zulip):

except it rewrites the whole history

eddyb (Apr 01 2020 at 12:25, on Zulip):

does git subrepo keep the commits or just make bland sync commits?

centril (Apr 01 2020 at 12:25, on Zulip):

@eddyb I have no problem with additional constraints you have :slight_smile:

oli (Apr 01 2020 at 12:25, on Zulip):

pushes keep the commits

eddyb (Apr 01 2020 at 12:25, on Zulip):

your clippy import PR definitely is missing the clippy history :P

oli (Apr 01 2020 at 12:25, on Zulip):

so the remote repo sees everything

eddyb (Apr 01 2020 at 12:26, on Zulip):

okay but also in the opposite direction?

oli (Apr 01 2020 at 12:26, on Zulip):

yea, but does rustc care about clippy commits?

eddyb (Apr 01 2020 at 12:26, on Zulip):

I don't want git blame to break

oli (Apr 01 2020 at 12:26, on Zulip):

subrepo discussion: https://github.com/ingydotnet/git-subrepo/issues/246

eddyb (Apr 01 2020 at 12:26, on Zulip):

git subtree pull seems to create one commit per remote commit unless you use --squash

oli (Apr 01 2020 at 12:27, on Zulip):

yea, unsupported right now in git subrepo

eddyb (Apr 01 2020 at 12:27, on Zulip):

so why would we use subrepo over subtree?

oli (Apr 01 2020 at 12:28, on Zulip):

idk

eddyb (Apr 01 2020 at 12:28, on Zulip):

anyway it turns out there's no tool that does the submodule thing but tracked transitively instead of opaquely

eddyb (Apr 01 2020 at 12:28, on Zulip):

heck opaque tracking would be fine if they could be pushed alongside eachother automatically ugh

eddyb (Apr 01 2020 at 12:29, on Zulip):

so the choices are:

oli (Apr 01 2020 at 12:29, on Zulip):

well, history could be kept with git subrepo, it just doesn't support it yet

oli (Apr 01 2020 at 12:30, on Zulip):

it's not a technical limitation

eddyb (Apr 01 2020 at 12:30, on Zulip):

I am suspicious of it existing when git subtree already does, is there an explainer somewhere?

eddyb (Apr 01 2020 at 12:30, on Zulip):

is it older or newer than git subtree?

centril (Apr 01 2020 at 12:32, on Zulip):

@oli btw, I think it would be good to first make a PR with the contributing notes before doing the move itself

centril (Apr 01 2020 at 12:32, on Zulip):

easier to review tweaks to the contributing notes that way

oli (Apr 01 2020 at 12:35, on Zulip):

subrepo is newer than subtree

oli (Apr 01 2020 at 12:35, on Zulip):

its docs explicitly mention subtree having problems that it solves

eddyb (Apr 01 2020 at 12:36, on Zulip):

which are?

oli (Apr 01 2020 at 12:37, on Zulip):

Fixes known rebase failures with git-subtree.

oli (Apr 01 2020 at 12:37, on Zulip):

https://github.com/ingydotnet/git-subrepo#benefits

oli (Apr 01 2020 at 12:40, on Zulip):

idk, reading https://stackoverflow.com/questions/32407634/when-to-use-git-subtree sounds to me like subtree has the same benefits

oli (Apr 01 2020 at 12:47, on Zulip):

note that subtree also does the "is basically monorepo" strategy

oli (Apr 01 2020 at 12:48, on Zulip):

I honestly don't care whether we use git subtree or git subrepo, although the former being part of git makes it more convenient

eddyb (Apr 01 2020 at 12:58, on Zulip):

@oli a monorepo with lost history is more of a terrible monorepo to me than one where we at least sync history both ways

eddyb (Apr 01 2020 at 13:00, on Zulip):

subrepo mentions "Your git history is kept squeaky clean."

eddyb (Apr 01 2020 at 13:00, on Zulip):

that's... not what I want

oli (Apr 01 2020 at 13:00, on Zulip):

you don't really lose history, with submodule you still need to go to the right commit on the other repo, git subrepo had these commit ids also stored in a file, just like git submodule

eddyb (Apr 01 2020 at 13:00, on Zulip):

"Upstream history (clone/pull) is condensed into a single commit."

oli (Apr 01 2020 at 13:00, on Zulip):

I'm convinced

oli (Apr 01 2020 at 13:00, on Zulip):

ignore git subrepo please

eddyb (Apr 01 2020 at 13:00, on Zulip):

lol

eddyb (Apr 01 2020 at 13:01, on Zulip):

I am trying to figure out the best option, and not be kneejerky, FWIW

oli (Apr 01 2020 at 13:01, on Zulip):

I just want any option that is monorepo style for users

oli (Apr 01 2020 at 13:01, on Zulip):

and I get that history is nice, and git subtree has nice (even if fake) history

oli (Apr 01 2020 at 13:01, on Zulip):

so let's do that

oli (Apr 01 2020 at 13:02, on Zulip):

these are the only three options I found (beyond full monorepo)

eddyb (Apr 01 2020 at 13:02, on Zulip):

3?

oli (Apr 01 2020 at 13:02, on Zulip):

I just never thought to check git subtree because git subrepo docs said it's not great

oli (Apr 01 2020 at 13:02, on Zulip):

submodule, subrepo, subtree, monorepo

eddyb (Apr 01 2020 at 13:02, on Zulip):

aaah

oli (Apr 01 2020 at 13:02, on Zulip):

and submodule and monorepo are not what we want

eddyb (Apr 01 2020 at 13:03, on Zulip):

I was gonna take subtree out for a spin, are you still on it then?

oli (Apr 01 2020 at 13:03, on Zulip):

already done, you have a PR assigned to you

eddyb (Apr 01 2020 at 13:03, on Zulip):

wait what

centril (Apr 01 2020 at 13:04, on Zulip):

@oli did you try the ./x.py check thing?

oli (Apr 01 2020 at 13:04, on Zulip):

no

centril (Apr 01 2020 at 13:04, on Zulip):

let's, please

oli (Apr 01 2020 at 13:04, on Zulip):

before r+ sure

centril (Apr 01 2020 at 13:04, on Zulip):

sure :slight_smile:

eddyb (Apr 01 2020 at 13:05, on Zulip):

oh, GH is nice enough to show your own Rust commits separately :D

simulacrum (Apr 01 2020 at 13:05, on Zulip):

(We should make x.py check changes a separate PR, there's no need to connect the two beyond a toggle that enables it by default when we merge the two)

centril (Apr 01 2020 at 13:05, on Zulip):

@simulacrum okay, but let's land that separate PR first then

simulacrum (Apr 01 2020 at 13:05, on Zulip):

...obviously, yes.

centril (Apr 01 2020 at 13:06, on Zulip):

heh wasn't obvious to me :P

eddyb (Apr 01 2020 at 13:06, on Zulip):

huh, are commit signatures preserved?

eddyb (Apr 01 2020 at 13:06, on Zulip):

that's a benefit I haven't considered

eddyb (Apr 01 2020 at 13:07, on Zulip):

@oli you won't believe this

eddyb (Apr 01 2020 at 13:07, on Zulip):

git subtree preserves commit hashes

eddyb (Apr 01 2020 at 13:07, on Zulip):

my face right now -> :open_mouth:

eddyb (Apr 01 2020 at 13:08, on Zulip):

https://github.com/rust-lang/rust-clippy/commits/master

oli (Apr 01 2020 at 13:08, on Zulip):

not sure that will be permanent

oli (Apr 01 2020 at 13:08, on Zulip):

unless it keeps merging..hmmm

eddyb (Apr 01 2020 at 13:08, on Zulip):

doesn't matter, most of the history is in the past

oli (Apr 01 2020 at 13:08, on Zulip):

won't be soon

oli (Apr 01 2020 at 13:08, on Zulip):

(if we do this)

eddyb (Apr 01 2020 at 13:09, on Zulip):

still, I had already accepted losing all commit hash correspondence

eddyb (Apr 01 2020 at 13:14, on Zulip):

okay so I think the explanation is that git subtree (ab)uses merge commits to hide adding the prefix

eddyb (Apr 01 2020 at 13:15, on Zulip):

meaning every git subtree pull should keep matching commits from the separate repo

eddyb (Apr 01 2020 at 13:15, on Zulip):

and probably every git subtree push too (but in that direction there might be pollution, and at least that part I don't mind being rewritten)

oli (Apr 01 2020 at 13:17, on Zulip):

we also need to be careful rebasing (as in never doing so) the merge commits created by git subtree

simulacrum (Apr 01 2020 at 13:17, on Zulip):

but that's only a concern for whoever creates the subtree-operation PRs, right?

eddyb (Apr 01 2020 at 13:17, on Zulip):

but that's only when sync-ing, right? I wonder if we should automate that

oli (Apr 01 2020 at 13:17, on Zulip):

yes

eddyb (Apr 01 2020 at 13:17, on Zulip):

kind of like creating a rollup PR

eddyb (Apr 01 2020 at 13:17, on Zulip):

you wouldn't rebase a rollup PR either

oli (Apr 01 2020 at 13:18, on Zulip):

we can automate later. This is still strictly an improvement over submodule

eddyb (Apr 01 2020 at 13:18, on Zulip):

right

eddyb (Apr 01 2020 at 13:18, on Zulip):

there are already issues where people accidentally include submodule reverts in their rebases

simulacrum (Apr 01 2020 at 13:19, on Zulip):

I think that won't be possible here

eddyb (Apr 01 2020 at 13:19, on Zulip):

now at least rebasing non-subtree changes shouldn't ever go wrong

eddyb (Apr 01 2020 at 13:20, on Zulip):

right, that's what I mean, we already had to be careful and I think now most people should have strictly less problems

bjorn3 (Apr 01 2020 at 13:20, on Zulip):

Will this give us (part of) the miri history inside rust-lang/rust twice? It already contains it once when the old const eval was replaced by the miri core.

eddyb (Apr 01 2020 at 13:20, on Zulip):

hilarious

simulacrum (Apr 01 2020 at 13:20, on Zulip):

my understanding is that subtree essentially means that no one has to know that clippy is a separate repo unless they want to

eddyb (Apr 01 2020 at 13:20, on Zulip):

they're the same commit hashes though

oli (Apr 01 2020 at 13:20, on Zulip):

I don't think so... I did it without changing commit hashes back then

eddyb (Apr 01 2020 at 13:20, on Zulip):

so it will look like rejoining a long-lostforgotten git history

oli (Apr 01 2020 at 13:21, on Zulip):

there'll just be a long part where we never saw any interaction between the mir branch and the rustc branch

oli (Apr 01 2020 at 13:21, on Zulip):

but that's trivial to check when we do the merge ;)

oli (Apr 01 2020 at 13:21, on Zulip):

(unless I kill github UI again, as I did for a bit with clippy earlier)

eddyb (Apr 01 2020 at 13:25, on Zulip):

@simulacrum do you want to review https://github.com/rust-lang/rust/pull/70655/commits/f8041069e27b9ab35c937255880c988ba0f2c362

eddyb (Apr 01 2020 at 13:25, on Zulip):

and https://github.com/rust-lang/rust/pull/70655/commits/6a05fee1933fe61e8615739a45eac7e2033ed4c9 I guess

simulacrum (Apr 01 2020 at 13:26, on Zulip):

They seem fine though that's no guarantee it'll work

eddyb (Apr 01 2020 at 13:27, on Zulip):

oh the second commit undos the removal of the toolstate in the first

eddyb (Apr 01 2020 at 13:28, on Zulip):

okay I think I see what's happening in rustbuild, looks fine to me too

eddyb (Apr 01 2020 at 13:36, on Zulip):

@oli are you working on ./x.py check support/testing btw?

eddyb (Apr 01 2020 at 13:56, on Zulip):

@oli @simulacrum oh, @edef explained it to me on IRC: git subtree uses git merge -Xsubtree internally, so it is technically a git feature, not just abusing the merge commit

oli (Apr 01 2020 at 13:56, on Zulip):

yes working on check

eddyb (Apr 01 2020 at 13:57, on Zulip):

and that's the only thing left before we throw this in, no FCP no MCP :P?

simulacrum (Apr 01 2020 at 13:57, on Zulip):

I would like to see the subrepo vs. subtree summary that Ralf asked for on the issue

simulacrum (Apr 01 2020 at 13:58, on Zulip):

and IMO we probably want to at least give it some time for responses, at the very least some mention at meeting tomorrow

simulacrum (Apr 01 2020 at 13:58, on Zulip):

I would FCP it personally

simulacrum (Apr 01 2020 at 13:58, on Zulip):

or at least "FCP without the final comment period"

eddyb (Apr 01 2020 at 13:59, on Zulip):

@nikomatsakis meeting sound good for final decision ^^?

nikomatsakis (Apr 01 2020 at 14:00, on Zulip):

Heh, glad to see the question raised.

centril (Apr 01 2020 at 14:01, on Zulip):

FCP for infra or compiler team?

nikomatsakis (Apr 01 2020 at 14:01, on Zulip):

I felt like there should be some "decision point" here as well.

simulacrum (Apr 01 2020 at 14:03, on Zulip):

definitely compiler, infra has no stake in this IMO.

nikomatsakis (Apr 01 2020 at 14:04, on Zulip):

I was going to say, I think it seems more compiler, and maybe dev-tools, but I am wondering a bit just what we're FCP'ing -- I think it is something like

centril (Apr 01 2020 at 14:04, on Zulip):

As long as all the infra works out for rollups and whatnot then T-release should have no issues too

nikomatsakis (Apr 01 2020 at 14:04, on Zulip):

At minimum the dev-tools folks should be pinged :)

nikomatsakis (Apr 01 2020 at 14:04, on Zulip):

since it seems like this effects their workflows?

centril (Apr 01 2020 at 14:04, on Zulip):

@nikomatsakis well Oliver and Manish, et. al are involved

centril (Apr 01 2020 at 14:05, on Zulip):

so that's the Clippy folks

oli (Apr 01 2020 at 14:06, on Zulip):

we want to test this with clippy, and clippy ppl know about it

oli (Apr 01 2020 at 14:06, on Zulip):

if the test is successfull, we try to convince other dev tools that this is cool and they should do it, too

eddyb (Apr 01 2020 at 14:06, on Zulip):

can't wait to stop having nightlies missing tools

centril (Apr 01 2020 at 14:06, on Zulip):

I want some time though for this to sink in before we move on to Clippy Miri

centril (Apr 01 2020 at 14:06, on Zulip):

to make sure the bors queue and stuff is running smoothly

eddyb (Apr 01 2020 at 14:07, on Zulip):

the status quo makes it really hard to rustup update without setting a bunch of nightly dates in various places

nikomatsakis (Apr 01 2020 at 14:09, on Zulip):

ok, seems good, I say we FCP w/ t-compiler -- as much to make people aware as anything -- and we can bring other tools in as we go by pinging the folks involved in each tool

eddyb (Apr 01 2020 at 14:09, on Zulip):

we want to do it on the issue, right?

eddyb (Apr 01 2020 at 14:09, on Zulip):

because the PR has 8000 commits on it :P

nikomatsakis (Apr 01 2020 at 14:09, on Zulip):

there's a PR that updates the docs

nikomatsakis (Apr 01 2020 at 14:10, on Zulip):

that is relatively short

eddyb (Apr 01 2020 at 14:10, on Zulip):

ah good, right

nikomatsakis (Apr 01 2020 at 14:10, on Zulip):

and I think hopefully clarifies what's happening

centril (Apr 01 2020 at 14:10, on Zulip):

I think the docs PR might want some tweaks though

centril (Apr 01 2020 at 14:10, on Zulip):

for the text and stuff

nikomatsakis (Apr 01 2020 at 14:10, on Zulip):

anyway I don't care that much where we fcp but the docs PR or issue seems fine, we should certainly point at the docs either way

nikomatsakis (Apr 01 2020 at 14:10, on Zulip):

and if they don't clarify what's going on, seems like a bug in the docs :)

eddyb (Apr 01 2020 at 15:53, on Zulip):

@oli can you test git subtree push?

oli (Apr 01 2020 at 15:54, on Zulip):

sure

eddyb (Apr 01 2020 at 15:54, on Zulip):

make a local change to src/tools/clippy, commit on anoli-obk/rust branch and push it to a branch on oli-obk/rust-clippy

eddyb (Apr 01 2020 at 15:54, on Zulip):

I want to see what the push commits look like compared to the original

eddyb (Apr 01 2020 at 15:55, on Zulip):

@oli bonus: make rustc changes in the same commit, so we see the splitting out of those

eddyb (Apr 01 2020 at 15:55, on Zulip):

I think this is useful for demonstrating the workflow and making sure we know what it is before merging anything

oli (Apr 01 2020 at 15:59, on Zulip):

I'm watching some number count up

oli (Apr 01 2020 at 15:59, on Zulip):

not sure what it does

oli (Apr 01 2020 at 15:59, on Zulip):

but it's at 3k now

eddyb (Apr 01 2020 at 16:00, on Zulip):

interesting :D

oli (Apr 01 2020 at 16:16, on Zulip):
~/P/r/r/w/rust4> time git subtree push git@github.com:rust-lang/rust-clippy.git test_subtree --prefix src/tools/clippy
git push using:  git@github.com:rust-lang/rust-clippy.git test_subtree
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 8 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 344 bytes | 344.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote:
remote: Create a pull request for 'test_subtree' on GitHub by visiting:
remote:      https://github.com/rust-lang/rust-clippy/pull/new/test_subtree
remote:
To github.com:rust-lang/rust-clippy.git
 * [new branch]              8921de4666bc959d9201679d15d91eea9155ca35 -> test_subtree
73.48user 106.07system 3:01.20elapsed 99%CPU (0avgtext+0avgdata 149376maxresident)k
576inputs+64776outputs (0major+40951273minor)pagefaults 0swaps
oli (Apr 01 2020 at 16:16, on Zulip):

https://github.com/rust-lang/rust-clippy/compare/test_subtree?expand=1

eddyb (Apr 01 2020 at 16:20, on Zulip):

@oli can you push the Rust counterpart to oli-obk/rust?

oli (Apr 01 2020 at 16:21, on Zulip):

there's no counterpart I think?

eddyb (Apr 01 2020 at 16:21, on Zulip):

no, I mean the commit you made locally

oli (Apr 01 2020 at 16:21, on Zulip):

oh

oli (Apr 01 2020 at 16:22, on Zulip):

https://github.com/oli-obk/rust/pull/new/subrepo_experiments

eddyb (Apr 01 2020 at 16:23, on Zulip):

https://github.com/oli-obk/rust/commits/subrepo_experiments
https://github.com/rust-lang/rust-clippy/commits/test_subtree

eddyb (Apr 01 2020 at 16:23, on Zulip):

https://github.com/oli-obk/rust/commit/ba54e7645904ba0e8ea5ef0d28858f8ed2114643
https://github.com/rust-lang/rust-clippy/commit/8921de4666bc959d9201679d15d91eea9155ca35

eddyb (Apr 01 2020 at 16:24, on Zulip):

so commits are rewritten and hashes change, meaning "subtree merge commits" only exist for pulling

eddyb (Apr 01 2020 at 16:24, on Zulip):

@RalfJ ^^ push experiment successful, we learnt something :)

eddyb (Apr 01 2020 at 16:24, on Zulip):

or confirmed, rather

eddyb (Apr 01 2020 at 16:25, on Zulip):

I think this is the correct thing, since we don't want to bloat the separate repos with full rust-lang/rust commits

RalfJ (Apr 01 2020 at 16:38, on Zulip):

I dont know what to look at^^

RalfJ (Apr 01 2020 at 16:39, on Zulip):

so that was a "subtree push"?

eddyb (Apr 01 2020 at 16:42, on Zulip):

yupp

eddyb (Apr 01 2020 at 16:42, on Zulip):

git subtree rewrote the commit to exclude the change outside of the directory, and adjusted the paths

RalfJ (Apr 01 2020 at 16:47, on Zulip):

:+1:

RalfJ (Apr 01 2020 at 16:47, on Zulip):

and then it becomes a normal PR on the target side, merged like anything else (conflict resolution etc)?

eddyb (Apr 01 2020 at 16:48, on Zulip):

I believe so

RalfJ (Apr 01 2020 at 16:48, on Zulip):

without having metadata, how does "subtree pull" figured out which part to merge?

eddyb (Apr 01 2020 at 16:48, on Zulip):

git doesn't need metadata either

RalfJ (Apr 01 2020 at 16:48, on Zulip):

I guess since it has the upstream commit IDs it can work with those? it has to make sure not to be confused by its own synced pushed stuff though

eddyb (Apr 01 2020 at 16:48, on Zulip):

or do you mean the prefix? I believe you still need to specify that?

eddyb (Apr 01 2020 at 16:49, on Zulip):

@RalfJ git itself can merge identical changes in distinct commits

eddyb (Apr 01 2020 at 16:49, on Zulip):

especially if the files are identical, they will be the same "object" (i.e. the files have the same hash, so they don't need merging)

eddyb (Apr 01 2020 at 16:50, on Zulip):

the hard part is telling git where the files are supposed to be, which git subtree pull apparently does via git merge -Xsubtree

eddyb (Apr 01 2020 at 16:52, on Zulip):

@oli can you try to git subtree pull the commit you pushed (on top of the local version of that commit), and push the resulting merge commit to another oli-obk/rust branch?

eddyb (Apr 01 2020 at 16:53, on Zulip):

if we want to be sure merges are straight-forward

oli (Apr 01 2020 at 16:55, on Zulip):

done

eddyb (Apr 01 2020 at 16:56, on Zulip):

do you never delete PR branches? damn there's a lot of them

eddyb (Apr 01 2020 at 16:56, on Zulip):

oh you used the same branch again?

oli (Apr 01 2020 at 16:56, on Zulip):

yea

eddyb (Apr 01 2020 at 16:57, on Zulip):

so git subtree created this? https://github.com/oli-obk/rust/commit/b4d4a85affdb1114ed2420299eaa6fa244f83500

oli (Apr 01 2020 at 16:57, on Zulip):

i don't delete branches, because once I realized I had so many, they were too many to clean up

eddyb (Apr 01 2020 at 16:57, on Zulip):

and it just succeeded automatically?

oli (Apr 01 2020 at 16:57, on Zulip):

yes

eddyb (Apr 01 2020 at 16:58, on Zulip):

@oli if you give me write access I can spend a few hours going through your merged PRs and removing them :P

oli (Apr 01 2020 at 16:58, on Zulip):

lol, I can just run a script for merged PRs

eddyb (Apr 01 2020 at 16:58, on Zulip):

okay yeah and the merge commit shows up as 0 changes because the two parents are identical

eddyb (Apr 01 2020 at 16:58, on Zulip):

I wish GitHub could show that one of the parents is actually in a sub-directory :D

eddyb (Apr 01 2020 at 16:58, on Zulip):

because then it would feel like a 100% git feature

oli (Apr 01 2020 at 16:59, on Zulip):

open an issue against github? :P

eddyb (Apr 01 2020 at 16:59, on Zulip):

I never know where to do that :P

eddyb (Apr 01 2020 at 16:59, on Zulip):

the other day I was bothered by their code blocks CSS breaking Unicode Box-Drawing characters

bjorn3 (Apr 01 2020 at 17:01, on Zulip):

There is https://github.community/ and https://support.github.com/contact

eddyb (Apr 01 2020 at 17:01, on Zulip):

the latter IME can take months to get a reply :(

RalfJ (May 02 2020 at 13:28, on Zulip):

Looks like this is actually finally landing :tada:

RalfJ (May 02 2020 at 13:29, on Zulip):

one question: AFAIK the "contributors" page for rust counts commit across repositories to count "contributions" of people. will this mean that commits get counted twice because of the subtree pull PRs? will @oli be leading that list forever because of 80k clippy commits :rofl: ?

bjorn3 (May 02 2020 at 14:09, on Zulip):

git subtree seems to preserve the original author and committer, so no.

simulacrum (May 02 2020 at 14:59, on Zulip):

hm, I think we count by commit sha

simulacrum (May 02 2020 at 14:59, on Zulip):

in theory should not actually cause counts to diverge much

simulacrum (May 02 2020 at 15:00, on Zulip):

since the commits aren't authored by oli (uniquely) and we already counted all clippy commits

RalfJ (May 03 2020 at 09:11, on Zulip):

if it dedups commits that would do it yes

RalfJ (May 03 2020 at 09:11, on Zulip):

well, mostly

RalfJ (May 03 2020 at 09:11, on Zulip):

sutree push will create sync commits in the tool repo that will have a different commit ID, I think

Last update: May 29 2020 at 16:55UTC