Make cherry pick error message more useful #33037
Conversation
|
Not everyone uses the |
build-system/common/git.js
Outdated
| @@ -179,7 +179,7 @@ function gitCommitterEmail() { | |||
| * @return {!Array<{sha: string, isCherryPick: boolean}>} | |||
| */ | |||
| function gitCherryMaster() { | |||
| return getStdout('git cherry master').trim().split('\n'); | |||
| return getStdout('git cherry upstream/master').trim().split('\n'); | |||
There was a problem hiding this comment.
Not everyone uses the origin / upstream method. I wonder if there's a way to specify "whichever remote is set to ampproject/amphtml"
This is true. On CI, there is no upstream remote. ampproject/amphtml:master is referred to as origin/master.
And for local development, it's not guaranteed that the remote ref for ampproject/amphtml:master actually contains all the latest commits. This is because the local repo might have an old view of that ref (e.g. the most recent fetch was a while ago).
I see that gitCherryMaster() is being used by build-system/compile/internal-version.js to set the correct version when a CP branch is being built. Is there a way to guarantee that the calling script fully syncs the local master branch to ampproject/amphtml:master before building? If we can do that, this change shouldn't be required.
There was a problem hiding this comment.
re: calling scripts - there's multiple points of entry, including gulp dist, gulp cherry-pick, and I just learned that gulp serve is one too. Not sure how to find all calling scripts.. and it would be cool to fix this specifically inside the function with no dependencies.
Here's a hacky but simple alternative, what do you think?
git remote add ${UNIQUE} git@github.com:ampproject/amphtml.git
git fetch ${UNIQUE} master
git cherry ${UNIQUE}/master
git remote remove ${UNIQUE}
There was a problem hiding this comment.
I don't think we should add inline steps here, that could then be triggered by every call-site that indirectly uses getVersion(), especially if the steps include an expensive git fetch operation.
A possible alternative is to write a new util function that compares the tip of the local master branch against the current tip of ampproject/amphtml:master. This can be done without any fetching, as follows:
# Tip of local master branch
git rev-parse master
# Tip of ampproject/amphtml:master
git ls-remote https://github.com/ampproject/amphtml.git refs/heads/masterIf they are not equal when a cherry-pick version is being built, then fail whatever workflow is doing the building. Not sure what workflow this would be, but I'd imagine we can target it to just the ones that matter. Would that solve the problem we're running into?
There was a problem hiding this comment.
Good point about about being fetch being expensive.
The workflows that matter are gulp cherrypick and gulp release which already sync local and remote. I guess this fix is for unblocking local development when master is out of date... kind of just leaning towards updating the error message here and calling it a day!
https://github.com/ampproject/amphtml/blob/master/build-system/compile/internal-version.js#L91
There was a problem hiding this comment.
Updating the error message SGTM.
Last thought I had in mind: What is the best signal to indicate that local master is outdated (without paying the tax of a git fetch?) Is gitCherryMaster().length > 999 guaranteed to be true? Or are there circumstances under which we could have an outdated master but that condition is false?
Anyhow, this bit can be figured out separately.
|
Hey @danielrozenberg! These files were changed: |
estherkim
changed the title
To help local dev