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

Provide a way to translate the PreReleaseLabel (alpha, beta etc) to a numeric value to avoid version collisions #1433

Conversation

ruhullahshah
Copy link
Contributor

Provides a way to translate the PreReleaseLabel to a numeric value in order to avoid version collisions across different branches. For example, a release branch created after "1.2.3-alpha.55" results in "1.2.3-beta.1" and thus e.g. "1.2.3-alpha.4" and "1.2.3-beta.4" would have the same file version: "1.2.3.4".

Related Issues 1145, 1366

@ruhullahshah ruhullahshah changed the title Provide a way to translate PreReleaseLabel (alpha, beta etc) to a numeric value to avoid version collisions Provide a way to translate the PreReleaseLabel (alpha, beta etc) to a numeric value to avoid version collisions Jun 28, 2018
@jbaehr
Copy link
Contributor

jbaehr commented Jul 12, 2018

What do you (and @BCSharp, @JakeGinnivan, @asbjornu) think about providing default values for the pre-release weight supporting the standard alpha->beta->final scheme? This PR offers a clean solution for those who know the problem, but it always requires additional configuration. I think the default configuration should be usable already, giving a consistent picture with the rest of the default configuration.
I'd vote for assigning a default weight to the default labels so that this feature offers additional benefit out of the box.

Now the question what to use as default weights. On Windows we have a 16-bit constraint we cannot do anything against. In addition, I'd like to have the weights as nice numbers in base-10, which allows easy back-mapping for humans. Next, I expect the majority of builds to come with alpha-labes, so I want to reserve roughly the half of the available number space for them. The other half can then be used for beta, (rc, as needed) and the final build. So here comes by proposal:

  • alpha: pre-release weight = 0 (develop branch)
  • beta: pre-release weight = 30_000 (hotfix and release branch)
  • final, aka no pre-release label: pre-release weight = 60_000 (master, tags)

For special requirements there is enough room to fit release candidates after the beta, allow more then one "final build" or even to assign a non-zero offset for alpha-builds to make them "heavier" then CI-builds from feature branches. However, such things go far beyond an easy to understand default config.

@ruhullahshah
Copy link
Contributor Author

@jbaehr I agree that providing a branch specific default pre-release-weight would be helpful. The scheme you proposed seems good to me but I would like to hear more from the maintainers ( @JakeGinnivan, @asbjornu) and @BCSharp on this and I will update the PR.

@BCSharp
Copy link

BCSharp commented Jul 25, 2018

I have mixed thoughts about providing default non-zero values in this PS.

On one hand, it is nice to have a reasonable behaviour offered out-of-the box that the suggested defaults would provide.
On the other hand, I have concerns about changing anything automatically for users that do not have those values configured and maybe depend on the current behaviour. A change like that is better saved for a major version upgrade, accompanied with release notes.

In the end, I would not do it in this PR. The PR as is now is more likely to be accepted by the maintainers, (since it does not change anything on the current behaviour unless explicitly configured) and having it merged is way more important than sensible defaults.

@jbaehr
Copy link
Contributor

jbaehr commented Jul 25, 2018

@BCSharp My proposal about sensible defaults for the branch-specific pre-release-weight is not about changing existing behavior, it's only about improving this PR.
Note that the existing variable PreReleaseNumber continuous to exist, keeping its semantics. Only the newly introduced variable WeightedPreReleaseNumber is affected, and with a weight defaulting to zero both are equivalent.

I'm explicitly not proposing "{Major}.{Minor}.{Patch}.{WeightedPreReleaseNumber}" as default for assembly-file-versioning-format, although this is exactly is required by Windows Installer. That would be a breaking change, indeed.

@BCSharp
Copy link

BCSharp commented Jul 26, 2018

@jbaehr Yes, indeed. So in this case, it makes more sense to have reasonable defaults right from the beginning.

@asbjornu
Copy link
Member

@ruhullahshah, sorry for my late input on this, but I agree with @jbaehr and @BCSharp in that reasonable defaults make a lot of sense. If you could add that and rebase this PR, I'd love to merge it and have it shipped as part of GitVersion 5.

@asbjornu asbjornu added this to the 5.0.0 milestone Feb 20, 2019
@ruhullahshah
Copy link
Contributor Author

@asbjornu I have updated the the PR with the requested changes.

@asbjornu
Copy link
Member

@ruhullahshah, awesome, thanks!

@jbaehr and @BCSharp, do you agree with the defaults provided in the PR? If so, this is good to be merged as soon as the tests turn green.

@jbaehr
Copy link
Contributor

jbaehr commented Feb 20, 2019

My proposal for defaults ("alpha":0, "beta":30_000, "<non, aka final>":60_000) was based on the assumption that the prerelease number is reset whenever the next label is reached. This holds true for "beta", but recently I've observed that the final build (without a label) has a value of all commits since the last final in it's prerelease number. Given this fact, 60_000 would overflow the 16-bit if we have more then good 5000 commits since the last release.
If there is a strong reason for the prerelease-number-reset to behave differently for different labels, we have to adjust the defaults. If I've encountered a bug, those defaults are fine (and we need to hunt down the bug)

@ruhullahshah
Copy link
Contributor Author

I suggest we decrease the defaults for <non, aka final> from 60,000 to 55,000 for now and then open a new PR for the observation reported by @jbaehr, if required. Also these are merely the defaults and can be tuned using the pre-release-weight.

@asbjornu
Copy link
Member

@ruhullahshah, sounds good. Can you implement the decrease and do a rebase on HEAD of master so we get rid of the merge commits? 🙏

@ruhullahshah ruhullahshah force-pushed the support_weighted_pre_release_number_per_branch branch 2 times, most recently from 2515f40 to 49e3650 Compare February 27, 2019 11:21
@ruhullahshah
Copy link
Contributor Author

@asbjornu Done

@asbjornu
Copy link
Member

Excellent, @ruhullahshah. But can you please repeat the rebase due to the merge of #1599? 😅

@ruhullahshah ruhullahshah force-pushed the support_weighted_pre_release_number_per_branch branch from 49e3650 to db30469 Compare February 27, 2019 12:12
@ruhullahshah
Copy link
Contributor Author

@asbjornu Done :D

@asbjornu asbjornu merged commit 7fce4e6 into GitTools:master Feb 27, 2019
@asbjornu
Copy link
Member

🙏

@ruhullahshah ruhullahshah deleted the support_weighted_pre_release_number_per_branch branch February 27, 2019 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants