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

Add Versioning Support from version.txt #3140

Merged
merged 31 commits into from
Feb 21, 2025

Conversation

ahnaf-tahmid-chowdhury
Copy link
Contributor

Overview

This pull request introduces a versioning mechanism that utilizes a version.txt file located in the tools folder. Both pyproject.toml and CMakeLists.txt have been updated to read version information from this file, ensuring consistency across the project.

Changes

  • Added version.txt: A new file in the tools directory containing the version string in the format X.X.X or X.X.X-any.

  • Updated pyproject.toml:

    • The version field in pyproject.toml is now set to read the version from version.txt. This enables automatic versioning based on the contents of version.txt.
  • Updated CMakeLists.txt:

    • The CMake configuration has been modified to read the version from version.txt and set the OPENMC_VERSION accordingly.
    • Implemented logic to handle versions with optional suffixes (e.g., -dev, -any), ensuring that the header files use only the main version number without the suffix.

Benefits

  • Consistency: Centralizing version information in a single file (version.txt) ensures that both Python and CMake projects remain in sync.
  • Flexibility: The version can be easily updated by modifying just one file, simplifying version management.

Fixs

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

Not sure how to resolve this

ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. 
If you have updated the package versions, please update the hashes. Otherwise, 
examine the package contents carefully; someone may have tampered with them.
unknown package:
    Expected sha256 f728bb61f43fce850d622ced3b3d51b3116f767685ca4e4e0076f624e2d2307d
         Got        afe0e1873a0a0858a245ccd771066eddc07d20068859f1dd669a002e5dc68a65

pyproject.toml Outdated
@@ -58,6 +58,9 @@ Documentation = "https://docs.openmc.org"
Repository = "https://github.com/openmc-dev/openmc"
Issues = "https://github.com/openmc-dev/openmc/issues"

[tool.setuptools.dynamic]
version = {file = "tools/version.txt"}
Copy link
Member

Choose a reason for hiding this comment

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

Does this file need locating elsewhere, perhaps in the source code. I'm not sure but perhaps that helps when packaging as I don't think the tools folder is included in the packaging (e.g. conda)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is only required at build time. If it is necessary at runtime, we can put it in the root dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that openmc-feedstock uses a specific release tar file, which includes all the folders, including the tools folder.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for answering this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an expert, but this doesn't feel like a tools sort of thing. To me it feels like a top level file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing it this way means openmc.__version__ still works due to how __version__ is populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have stored this file in the tools directory because it is only required during build time. Python's wheel uses it to create the wheel, and CMake uses it to generate the version.h file. Additionally, we can use this file for other purposes. I placed it in that folder to keep the root directory clean. However, if necessary, we can move it to the root directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree that this is more of a root-level file.

@shimwell
Copy link
Member

The CI passed when I rerun a single job just now. So I have re triggered the others and I guess it will pass now. I think the previous failure was unrelated to this CI

@shimwell
Copy link
Member

shimwell commented Sep 26, 2024

CI is all green 🎉

Do you think it is worth adding a version test to the pytests, perhaps something like this
https://github.com/fusion-energy/cad_to_dagmc/blob/main/tests/test_version.py

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

I have already added something similar to this in the CMakeLists.txt, which will perform a similar task during configuration. I am also using setuptools to get the version number on the Python side, which will cause an error if an improper format is set.

@shimwell
Copy link
Member

This looks good to me, does anyone else want to take a peak. Perhaps @MicahGale

Copy link
Contributor

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

Design looks good. However this doesn't seem to be propagating to the C++ side properly in my testing.

I followed the build from source method cmake .. && make. However build/bin/openmc -v was still showing openmc version 0.15.0.

pyproject.toml Outdated
@@ -58,6 +58,9 @@ Documentation = "https://docs.openmc.org"
Repository = "https://github.com/openmc-dev/openmc"
Issues = "https://github.com/openmc-dev/openmc/issues"

[tool.setuptools.dynamic]
version = {file = "tools/version.txt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an expert, but this doesn't feel like a tools sort of thing. To me it feels like a top level file.

@@ -0,0 +1 @@
1.15.1-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably beyond the scope of this PR alone, but this isn't PyPA compliant. It should be: 0.15.1.devN (Probably dev1?). Also you accidentally incremented a major release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setuptools will handle the task for us, so we can continue using the legacy format.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something setuptools does because it's nice to do; not because it has to. I think changing to PyPA compliant is a small change that would reduce the risk of something breaking in the future. (Honestly I don't think many people would even notice beyond some of the core devs.)

Copy link
Contributor

Choose a reason for hiding this comment

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

PyPA does allow -dev but probably better to use a dot since it's perferred

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. TIL.

pyproject.toml Outdated
@@ -58,6 +58,9 @@ Documentation = "https://docs.openmc.org"
Repository = "https://github.com/openmc-dev/openmc"
Issues = "https://github.com/openmc-dev/openmc/issues"

[tool.setuptools.dynamic]
version = {file = "tools/version.txt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing it this way means openmc.__version__ still works due to how __version__ is populated.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

I followed the build from source method cmake .. && make. However build/bin/openmc -v was still showing openmc version 0.15.0.

It seems you have an old version of openmc installed. And you have exported that on your PATH. You can easily review it by calling

which openmc

@MicahGale
Copy link
Contributor

I followed the build from source method cmake .. && make. However build/bin/openmc -v was still showing openmc version 0.15.0.

It seems you have an old version of openmc installed. And you have exported that on your PATH. You can easily review it by calling

which openmc

Oh yep that was my issue. I'm used to python where $PATH does not shadow the current directory.

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Adding a suggestion for convenience.

pyproject.toml Outdated
@@ -58,6 +58,9 @@ Documentation = "https://docs.openmc.org"
Repository = "https://github.com/openmc-dev/openmc"
Issues = "https://github.com/openmc-dev/openmc/issues"

[tool.setuptools.dynamic]
version = {file = "tools/version.txt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree that this is more of a root-level file.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

A few immediate thoughts:

  • Some people argue that you shouldn't store version information in files in the first place. If there's a way we can do this that automatically pulls information from git, that would probably be better.
  • If we do go with what you have here, there's one more instance of hardcoding of the version in docs/source/conf.py.

@MicahGale
Copy link
Contributor

MicahGale commented Sep 27, 2024

  • Some people argue that you shouldn't store version information in files in the first place. If there's a way we can do this that automatically pulls information from git. If that case can be addressed I really prefer using git tag.

@paulromano: @gonuke brought up some good points, that there are use cases for when the source code may be used while detached from git. I'm not sure right now of a work-around for that case using git based methods, which I generally agree would be the ideal solution.

If we do go with what you have here, there's one more instance of hardcoding of the version in docs/source/conf.py.

I'd recommend that @ahnaf-tahmid-chowdhury go with using a importlib.... call in this file no matter what because that will be agnostic to however python retrieves version information.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

I have added the get_version_info function to extract the version info, but I am unsure what working directory Read the Docs is using. For now, I am assuming it is using docs/source/ as the working directory.

Recently, I noticed @MicahGale's comment on this. We can use importlib, but again, I need to understand how Read the Docs processes this. If it runs the conf.py file before installing OpenMC, it will fail.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

It seems docs buils are passing https://readthedocs.org/projects/openmc/builds/25766475/

We can use importlib.metadata.version, but we still need to use the get_version_info function as there are two var.

@gonuke
Copy link
Contributor

gonuke commented Oct 8, 2024

We definitely came here, @paulromano , to get broader perspective on this issue. I definitely like the elegance of git being a single source of truth, but worry about folks who build from tarballs (e.g. those generated automatically by GitHub upon release) rather than clones. Do we ignore them? Or do we find a way to make that work, too?

@MicahGale
Copy link
Contributor

@gonuke one thing I thought about is that setuptools-scm will create a _version.py file. As part of the GH release process a source tar ball with a version file could be included. I think building from a source tar ball for a specific release would be a lot more common than from a specific branch.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

An additional note: If we use setuptools-scm, when someone installs OpenMC from a tarball, it will build a version labeled 0.0.0.

@MicahGale
Copy link
Contributor

@ahnaf-tahmid-chowdhury Good point. Do you want to try and see if we could make a CI release that would embed a version file in a release tar ball from setuptools-scm?

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

For this, we may need to use a workflow to build OpenMC that will create the version file. Once done, it will push the updated commit to the develop branch. We can use any existing workflow, like this one, and it will only run if the PR gets merged. Let me know if I should proceed with that approach.

@MicahGale
Copy link
Contributor

No this is a bad idea. _version.py should never be in git. What I was thinking was as part of the release process having python -m build . create the version file. Create a complete source tar ball which include _version.py and then upload that to the GH release as an artifact.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

We are currently working on PR #3087, which will create wheel (.whl) artifacts after a release. Your approach is good, but GitHub automatically creates its own tarball with each release. So, in the end, we will be releasing multiple tarballs. And, I am unfamiliar with resolving that.

Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

A little typo....?

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks for all the work put into this @ahnaf-tahmid-chowdhury!

@paulromano
Copy link
Contributor

@pshriwise @gonuke @MicahGale are you all on board for merging this PR?

Copy link
Contributor

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

I verified that git_archival.txt still works with setuptools_scm, which is done. Let's ship this!

Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

A couple of final little comments...

Co-authored-by: Ahnaf Tahmid Chowdhury <ahnaf-tahmid@outlook.com>
@paulromano paulromano added this to the v0.15.1 milestone Feb 21, 2025
Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This looks good to me - thanks @ahnaf-tahmid-chowdhury for your work on this, looking forward to translating it to other projects

@paulromano paulromano merged commit a2a5c2a into openmc-dev:develop Feb 21, 2025
16 checks passed
@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

I just noticed that the Dockerfile is failing because it runs git clone with the --depth 1 option. We could add a method in the CMake file to fetch everything, but I wonder how setuptools_scm will handle that in the future when we switch to scikit-build-core. Or, we might consider removing the --depth option.

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.

6 participants