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 git rev to local version identifier #3568

Merged
merged 11 commits into from Dec 10, 2023

Conversation

toddstrader
Copy link
Contributor

Adds the Git rev to the local version label. There's a chicken-and-egg problem wrt PR numbers and creating news fragments. I'll add that next. Also, should I add a test for this? I guess I could check that the + exists and there's something git rev-like if it's a dev version?

closes #3562

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (40a27c2) 66.68% compared to head (1baf999) 67.53%.
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3568      +/-   ##
==========================================
+ Coverage   66.68%   67.53%   +0.85%     
==========================================
  Files          48       47       -1     
  Lines        8491     8514      +23     
  Branches     2413     2423      +10     
==========================================
+ Hits         5662     5750      +88     
+ Misses       1696     1632      -64     
+ Partials     1133     1132       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

setup.py Show resolved Hide resolved
@marlonjames
Copy link
Contributor

@toddstrader
Copy link
Contributor Author

I wonder why Python 3.6 doesn't work.

https://github.com/cocotb/cocotb/actions/runs/7143704333/job/19455778417?pr=3568#step:23:65

Huh, dunno yet. But I guess that answers my question about needing a test.

@toddstrader
Copy link
Contributor Author

toddstrader commented Dec 8, 2023

3.6 was failing because text was too new for subprocess:

Changed in version 3.7: Added the text parameter, as a more understandable alias of universal_newlines. Added the capture_output parameter.

I confirmed that the new test caught this issue and then resolved it.

@toddstrader
Copy link
Contributor Author

Also additional confirmation that things are working in 3.6 now:
https://github.com/cocotb/cocotb/actions/runs/7145071037/job/19459990612?pr=3568#step:23:65

@marlonjames marlonjames added the category:packaging issues related to (PyPi) packaging, etc. label Dec 8, 2023
@ktbarrett
Copy link
Member

Gentle reminder on the ready for review.

@toddstrader toddstrader marked this pull request as ready for review December 8, 2023 21:28
@toddstrader
Copy link
Contributor Author

Gentle reminder on the ready for review.

I respond well to loud yelling.

@ktbarrett
Copy link
Member

GOOD JOB, YOU DID IT!@

@ktbarrett
Copy link
Member

Reading deeper into PEP 440:

https://peps.python.org/pep-0440/#final-releases

When comparing release segments with different numbers of components, the shorter segment is padded out with additional zeros as necessary.

I read this as meaning 2.0.0.dev0 and 2.0.dev0 are the same. If this is a point of contention, I'm willing to concede.

@marlonjames
Copy link
Contributor

I don't really care 2.0 vs 2.0.0, I think it was probably to match the 3 levels we have used since 1.2.0.

@toddstrader
Copy link
Contributor Author

OK, so what's the verdict? Back to 2.0.0?

@ktbarrett
Copy link
Member

Yeah, sorry.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Member

@imphil imphil 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 your pull request, @toddstrader.

There's a chicken-and-egg problem wrt PR numbers and creating news fragments.

You can use the issue number instead of the PR number for the newsfragment.

@imphil imphil merged commit 8728fa7 into cocotb:master Dec 10, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:packaging issues related to (PyPi) packaging, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thoughts on adding git rev to dev version numbers?
4 participants