Skip to content

Cleanup and relocate BUILD_VERSION calculation #1382

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

Conversation

vector-of-bool
Copy link
Contributor

  • The computation of BUILD_VERSION is now lifted above project(), which allows us to use that value in the project() call itself, giving us correct PROJECT_VERSION values throughout.
  • Define BUILD_VERSION using mongo_setting(), and use the calc_release_version.py to compute its default value.

Possible concerns:

  • The LoadVersion and ParseVersion are extremely similar, but both are required to get the project to work. Some locations refer to the _VERSION_PATCH suffix, while others use _MICRO_VERSION suffix. These could/should probably be consolidated to a single variable set.
  • It isn't clear what the purpose of BUILD_VERSION (as a cache variable) serves when VERSION_CURRENT (the text file) will usually take precedence.

- The computation of BUILD_VERSION is now lifted above project(), which allows
  us to use that value in the project() call itself, giving us correct
  PROJECT_VERSION values throughout.
- Define BUILD_VERSION using mongo_setting(), and use the
  calc_release_version.py to compute its default value.
@rcsanchez97
Copy link
Contributor

OK, so before I review the changes, I will try to address some of your overall questions/concerns. A little bit of historical context is probably useful here.

In the beginning (of the libbson and mongo-c-driver projects), releases were made as tarballs (which you well know from your recent work on that part of the build). Knowledge of the current release version (and previous release version) was necessary for the build to produce correct results (i.e., so that it could populate the release version in one of the headers, and so that it could generate correct documentation for the current version and links to prior version documentation). In a release tarball that lacked Git history, the way to accomplish this was by creating the files VERSION_CURRENT and VERSION_PREVIOUS and shipping them in the release tarball.

When the packaging for RPM and DEB was done, it was necessary to determine the version number from the associated package-specific metadata (RPM spec file and debian/changelog) and then pass that value to the build. This was particularly necessary for the Debian build, which was executed from Git and which generated a tarball that did not have the VERSION_CURRENT (or VERSION_PREVIOUS) file. (Note that for both the RPM and DEB snapshot builds that we execute in Evergreen, we actually use calc_release_version.py since those builds are "ahead" of the corresponding packaging releases and we can't rely on the changelogs to have the correct version number.)

My recollection is that the solution was to implement the BUILD_VERSION variable with logic roughly along these lines:

  • if BUILD_VERSION was specified on the CMake command line, use that value
  • if BUILD_VERSION was not specified, then attempt to determine its value from VERSION_CURRENT (if it exists)
  • if VERSION_CURRENT doesn't exist, attempt to determine the version by calling calc_release_version.py (which, BTW, would only work in a directory that is a clone of the Git repo and which has enough depth of history to find the appropriate tags)
  • if the version came from somewhere other than the VERSION_CURRENT file (i.e., BUILD_VERSION was specified on the command line, or we had to resort to calling calc_release_version.py) then save the value in VERSION_CURRENT for later use by CMake invocations in the same build tree that might omit BUILD_VERSION from the command line

This rough logical outline reflects what I believe is the final/current state. It bears mentioning that we arrived here as a result of evolving the process through various steps. Some of the conditions which motivated some previous conditions are no longer relevant.

I have no problem with completely reworking how we handle informing the build of what version is being built, since given what has changed, it seems likely we could use a much simpler approach to achieve the necessary result.

Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like the simplification you've implemented here. In light of my overall comment on the PR, I would absolutely be in favor of a more comprehensive rethinking/reworking of how we handle the version (i.e., especially given that the release tarball is no longer a thing).

Also, when you merge this (or if you decide on a more comprehensive reworking, after you merge that) could you implement corresponding changes in the C++ driver and libmongocrypt so that we don't end up with divergent ways of handling the build version?

@rcsanchez97
Copy link
Contributor

@vector-of-bool One thing I forgot to mention is that you have to be careful about Python 2 -> 3. That is, your change doesn't seem to make provision for running on a platform that has only Python 2. In the libmongocrypt project (with which these changes will have to be synced at some point), we still build on RHEL 6.2, which has Python 2.6. The approach that we take still needs to work there.

@vector-of-bool
Copy link
Contributor Author

Both this and #1383 can be tweaked to work with Python 2.6, so that requirement is no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants