search

dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
659 stars 333 forks source link

eng/common is frequent source of merge conflicts #1576

Open ryanbrandenburg opened 5 years ago

ryanbrandenburg commented 5 years ago
  1. I clone an Arcade repo at version x.
  2. I need a fix from version y, so I do darc update-dependencies.
  3. the master branch automatically updates to version z (skipping y).
  4. I try to merge master back into my branch.
  5. Merge conflicts happen all over eng/common.

We dealt with similar problems in KoreBuild and our solution was to minimize the amount of "boilerplate" checked into the repo by downloading it as a zip to a known location. Then the only boilerplate needed was the code to download things, which doesn't change very often.

CC @natemcmaster who I've discussed this with before.

markwilkie commented 5 years ago

Can you explain a bit more how you see downloading a zip from a known location solves this? The merge still has to happen somewhere... Are you suggesting that because the file is not directly checked in that the dev does a manual merge of sorts?

For context, I'm taking this quite seriously as you guys have gone down this road and I'd really like to learn...

natemcmaster commented 5 years ago

Can you explain a bit more how you see downloading a zip from a known location solves this?

With KoreBuild, you really only need two scripts to use KoreBuild: build.sh and build.ps1, each < 300 lines of code. These rarely changed.

The piece that often changed was korebuild-lock.txt. This was used to identify which version of KoreBuild should be downloaded as a zip. Example of the korebuild lockfile: https://github.com/aspnet/AspNetCore/blob/6d2f2483d21b6ab13ce4f273110f26dc89af12e6/korebuild-lock.txt

version:3.0.0-build-20181206.3
commithash:96a199c48729398e013355167c21dd61ff0c574c

Korebuild-lock.txt often has merge conflicts, but because this lockfile is 2 lines, it's trivial to resolve.

With eng/common/ on the other hand, every repo that branches in any way (for servicing, for PRs, or otherwise) will eventually have to tango with merge conflcits. Because eng/common/ is now several thousand lines of code, I expect this is going to be painful in the long run.

ryanbrandenburg commented 5 years ago

What Nate said. The equivalent of KoreBuild-lock.txt for Arcade is probably Versions.Details.xml.

Just to clarify my suggestion: Instead of copying all the files in eng/common into every repo that uses Arcade, copy a single "bootstrap" script (well, one ps1 and one sh) into eng/, and that script copies the files currently in eng/common from the internet somewhere into a local cache which isn't checked in. Now there can't be merge conflicts on the CODE of Arcade, because it isn't checked in. Only on the version, which as Nate said is much easier to manage, and can't every result in a mix of versions.

Another pitfall this avoids: supposing a general dev who doesn't work on the build very often is told "You need version x of Arcade with such and such fix. See, that's what we did in this repo." The dev then copies these lines from the indicated repo into the one they're working on (assuming that's all that matters) and moves on with their life. Now they have a new SDK, but an old copy of eng/common, and they'll run into who knows what kind of issues until a new version of Arcade is published, because the Version.Details.xml reports that they have version x and therefor won't try to update eng/common even if they run darc update-dependencies at a later date. I actually already made that mistake before I understood Arcade better.

markwilkie commented 5 years ago

I see - you solved this with a lock file. Got it, and I understand the thinking.

@ryanbrandenburg - for what it's worth, the idea we came up with at the last meeting we had about this was to at a minimum 'verify' that the files hadn't been tampered with to at least alert the dev that they need to resolve their hand-modified files. https://github.com/dotnet/arcade-services/issues/2575

Thanks for the data - we do need to think through this and my thinking is that we'll circle back around to this as part of our "phase III" (post Preview 2) work.