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

Fix strange usage of pycbc.version.release #5033

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

titodalcanton
Copy link
Contributor

This fixes a couple minor weird things I noticed about how we report PyCBC's version in a couple places.

  1. In setup.py, we always set vinfo.release to a string like 'False' or 'True' which I always found odd. Why not just a bool? Here I turn it into a bool.
  2. While debugging Move to igwn-ligolw #5019 I noticed that the PyCBC version string reported on GraceDB is "Using PyCBC version 0.0a9071 (release)" which is wrong. This happens because of point 1, as the code that sets up that string assumes release is bool.
  3. In a recent test run of PyGRB, PyCBC's version information page says "ID : 0.0a9055" and "Version : False", both of which make no sense to me; for context, for the other libraries "ID" is the git hash. This is also fixed to show the git hash in "ID" and to give the actual version, plus the optional string "(release)" if appropriate, in "Version". This makes the "Version" line consistent with what is uploaded on GraceDB.
  • The author of this pull request confirms they will adhere to the code of conduct

Copy link
Member

@ahnitz ahnitz left a comment

Choose a reason for hiding this comment

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

This seems OK to me. I think confirmation of a couple checks besides the automated build would be useful though. Have you manually tried to make a release and non-release wheel offline and installed both? If so, then I think this can be merged.

@titodalcanton
Copy link
Contributor Author

No I have not made any manual tests, but I should.

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