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

cmd/dist: delete branchtag and its misleading use in findgoversion #42345

Closed
dmitshur opened this issue Nov 2, 2020 · 6 comments
Closed

cmd/dist: delete branchtag and its misleading use in findgoversion #42345

dmitshur opened this issue Nov 2, 2020 · 6 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Nov 2, 2020

The cmd/dist script contains a branchtag helper and the following code that uses it in findgoversion:

// If we're on a release branch, use the closest matching tag
// that is on the release branch (and not on the master branch).
if strings.HasPrefix(branch, "release-branch.") {
	tag, precise = branchtag(branch)
}

This comments suggests a certain behavior. As far as I can tell, this behavior happens almost never: release branches have always had a VERSION file checked in (going back as far as release-branch.go1 from 8 years ago), so execution inside findgoversion function won't reach that if statement. The only way I see for tag and/or precise to be anything other than their initial values are if one checks out a release branch and intentionally deletes the committed VERSION file, then tries to build Go from source.

(Perhaps this implementation worked better before the Go project transitioned from Mercurial to Git in commit f33fc0e, but I can't make sense of how it would have any effect since then.)

This issue is to investigate if my analysis is missing something. We may need to change this behavior in the future as part of work to make the development version string convey more accurate version information. This issue is would be relevant if we were to decide to change the release process to not keep the VERSION checked in on release branches (i.e., add it in one commit, remove in the next commit), but that seems unlikely.

If I'm missing something, please comment. Otherwise, I think we can delete the misleading nearly-dead code when the tree opens for Go 1.17 development.

CC @golang/release, @mvdan, @jayconrod, @dsymonds (in case you're still familiar this).

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 2, 2020
@dmitshur dmitshur added this to the Go1.17 milestone Nov 2, 2020
@dmitshur
Copy link
Contributor Author

dmitshur commented May 10, 2021

I think we can delete this as a next step, which shouldn't change behavior in meaningful ways, but make the current code easier to maintain and change in the future.

Better to do it not during the freeze, so moving to next cycle.

@dmitshur dmitshur modified the milestones: Go1.17, Go1.18 May 10, 2021
@dmitshur dmitshur added early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 10, 2021
@dmitshur dmitshur changed the title cmd/dist: branchtag and its use in findgoversion are misleading cmd/dist: delete branchtag and its misleading use in findgoversion May 10, 2021
@dmitshur dmitshur self-assigned this May 10, 2021
@cagedmantis
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.18.
That time is now, so this is a friendly ping so the issue is looked at again.

@dmitshur
Copy link
Contributor Author

I didn't get to this minor cleanup task in 1.18 cycle, will try next one.

@dmitshur dmitshur modified the milestones: Go1.18, Go1.19 Nov 12, 2021
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.19.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link

Change https://go.dev/cl/393954 mentions this issue: cmd/dist: delete special case for release branches without VERSION

@dmitshur
Copy link
Contributor Author

Had a chance to look at this now. I see now this makes only a tiny dent, but sent a CL anyway.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants