-
Notifications
You must be signed in to change notification settings - Fork 503
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
base: develop
Are you sure you want to change the base?
Add Versioning Support from version.txt
#3140
Conversation
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"} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for answering this
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
CI is all green 🎉 Do you think it is worth adding a version test to the pytests, perhaps something like this |
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. |
This looks good to me, does anyone else want to take a peak. Perhaps @MicahGale |
There was a problem hiding this 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"} |
There was a problem hiding this comment.
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.
tools/version.txt
Outdated
@@ -0,0 +1 @@ | |||
1.15.1-dev |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"} |
There was a problem hiding this comment.
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.
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
|
Oh yep that was my issue. I'm used to python where |
There was a problem hiding this 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"} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
.
@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
I'd recommend that @ahnaf-tahmid-chowdhury go with using a |
I have added the Recently, I noticed @MicahGale's comment on this. We can use |
It seems docs buils are passing https://readthedocs.org/projects/openmc/builds/25766475/ We can use |
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? |
@gonuke one thing I thought about is that |
An additional note: If we use |
@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 |
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. |
No this is a bad idea. |
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. |
It looks like the GH release GHA allows you to overwrite any artifacts, so I think we could just overwrite the default tar ball... hopefully. |
I’m still unsure if there is a GHA available that allows modification of the Source code (zip) and Source code (tar.gz) files created by GitHub during a release. If such an action exists, it might involve pushing a commit to the branch and then making the release. I’d be grateful for any guidance on this. |
In the past I've just used the |
But also it seems like we are really going down a rabbit hole of how to get this to work via git. I think this is approaching a point where perfect is the enemy of good. I personally think it would be good to move forward with a version file for the time being, and open a separate issue to migrate this to git, because making the release process work properly is becoming non-trivial. |
I just stumbled across the fact that |
069e0bf
to
d6be0ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some work before it becomes effective. First I tried to test how this worked with downloading code from github. It didn't work because you do not have any of the tags on your fork. Could you please push these tags to your fork?
Secondly, AFAICT, python isn't getting the version information very effectively; if at all. The ideal situation is for __version__
to be read from _version.py
. You can see how I implemented this before: https://github.com/idaholab/MontePy/blob/develop/montepy/__init__.py.
Finally, I think some tests will likely be needed to make sure this is robust. I'm thinking:
- ensure that
cmake
always builds with the same version assetuptools_scm
provides - ensure that the python api gets a version in all circumstances
- Esnure that a git archive has a version attached to it.
# Try to get the version from git describe | ||
if(EXISTS "${CMAKE_SOURCE_DIR}/.git") | ||
execute_process( | ||
COMMAND git describe --tags --match "*[0-9]*" --dirty | ||
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} | ||
OUTPUT_VARIABLE VERSION_STRING | ||
OUTPUT_STRIP_TRAILING_WHITESPACE | ||
) | ||
else() | ||
set(GIT_ARCHIVAL_FILE "${CMAKE_SOURCE_DIR}/.git_archival.txt") | ||
if(EXISTS "${GIT_ARCHIVAL_FILE}") | ||
file(READ "${GIT_ARCHIVAL_FILE}" GIT_ARCHIVAL_CONTENT) | ||
|
||
# Extract the describe-name line | ||
string(REGEX MATCH "describe-name: ([^\\n]+)" VERSION_STRING "${GIT_ARCHIVAL_CONTENT}") | ||
|
||
# If a match is found, extract the actual version | ||
if(VERSION_STRING MATCHES "describe-name: (.*)") | ||
set(VERSION_STRING "${CMAKE_MATCH_1}") | ||
else() | ||
message(FATAL_ERROR "Could not extract version from .git_archival.txt") | ||
endif() | ||
else() | ||
message(FATAL_ERROR "Neither git describe nor .git_archival.txt is available for versioning.") | ||
endif() | ||
endif() | ||
|
||
# Ensure the version string matches a standard format | ||
if(VERSION_STRING MATCHES "^v?([0-9]+\\.[0-9]+\\.[0-9]+)([-].*)?") | ||
set(VERSION_NO_SUFFIX "${CMAKE_MATCH_1}") | ||
else() | ||
message(FATAL_ERROR "Invalid version format: ${VERSION_STRING}") | ||
endif() | ||
|
||
# Set OpenMC version | ||
string(REPLACE "." ";" VERSION_LIST "${VERSION_NO_SUFFIX}") | ||
list(GET VERSION_LIST 0 OPENMC_VERSION_MAJOR) | ||
list(GET VERSION_LIST 1 OPENMC_VERSION_MINOR) | ||
list(GET VERSION_LIST 2 OPENMC_VERSION_RELEASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complicated and risks getting a different version. couldn't you just run python -m setuptools_scm
? This would require the package be installed though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using a validator in line 32, so if anything goes wrong, it will fail.
Your approach requires both Python and setuptools_scm to be installed first. For now, we can build OpenMC without the Python API. However, if this method is recommended here, we can adopt it.
Since we are using
Unfortunately, it will never be the same on develop branch, but always work on a specific tag. The Python version has an extra surfix (.dev*) in the development branch, but the |
I have created a release in my repo and found that the version info is written in the |
I'm still looking into this but I'm worried that doing it this way will always make a runtime call to
Ok that makes sense to drop the |
Let me explain how the metadata works. Whenever we install something with pip, it always comes with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, but testing is still needed to ensure all edge cases are handled. See my previous comment on some of these.
I dug into this a little bit more. One clarification is needed: |
As I mentioned, it is quite hard to achieve here, or we may need to change the OpenMC header so that it can work with strings. After that, we need to implement the
For now, if the format is not supported, the build will fail. So, the test is actually implemented during the configuration stage.
I think we can rely on
I believe this is an initial-time test that I have already run on my branch while releasing a version. Creating a dedicated test for this may consume resources and increase the time required for processing tests. I think there is no need to test this until we modify the |
It is always recommended to install Python packages in editable mode for developers, and this is also mentioned in the OpenMC developer docs. So, once OpenMC is installed, even in development mode, it comes with the METADATA file. |
Yes, developers may need to periodically update the OpenMC installation; otherwise, the METADATA will always contain the last installed version. I think we can add logic where, if someone tries to use OpenMC directly from the source, it will prompt them with a message like, 'Hey, please install OpenMC and run it outside the source root directory.' Let me know if this approach is feasible. |
I think the issue though is should it just return a version number, or the version number of the code that you are running right now always. The more I think about it needs to always return the version of what is actively running for trace-ability. |
Even if we include the |
Please note that OpenMC is already using the METADATA to extract version information, so I haven’t changed anything in that regard. I think @paulromano or @pshriwise might have some insights on how to proceed, or we are good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending @paulromano @pshriwise decision on the meta-data question.
Overview
This pull request introduces a versioning mechanism that utilizes a
version.txt
file located in thetools
folder. Bothpyproject.toml
andCMakeLists.txt
have been updated to read version information from this file, ensuring consistency across the project.Changes
Added
version.txt
: A new file in thetools
directory containing the version string in the formatX.X.X
orX.X.X-any
.Updated
pyproject.toml
:pyproject.toml
is now set to read the version fromversion.txt
. This enables automatic versioning based on the contents ofversion.txt
.Updated
CMakeLists.txt
:version.txt
and set theOPENMC_VERSION
accordingly.-dev
,-any
), ensuring that the header files use only the main version number without the suffix.Benefits
version.txt
) ensures that both Python and CMake projects remain in sync.Fixs