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

Strip leading zeroes from version #21532

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Jun 6, 2024

When setting the Drake version automatically, strip leading zeroes in order to maintain PEP 440 compliance. (Note that this logic is copied from our CI drivers, which were already doing this.)


This change is Reviewable

When setting the Drake version automatically, strip leading zeroes in
order to maintain PEP 440 compliance. (Note that this logic is copied
from our CI drivers, which were already doing this.)
@mwoehlke-kitware
Copy link
Contributor Author

+@jwnimmer-tri for review, please.

Release notes are something like "fixed a bug where user¹ builds of drake would have an invalid version identifier if built before 10:00 local time". (¹ "user" means building from source on a user's machine, as opposed to using pre-built binaries built by CI.)

I'm not sure if there's a good way to test this via CI.

@jwnimmer-tri jwnimmer-tri added release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html and removed release notes: fix This pull request contains fixes (no new features) labels Jun 6, 2024
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

We don't need release notes since the buggy PR never made it into any release.

-(release notes: fix) +(release notes: none)

:lgtm: both +(status: single reviewer ok)

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee jwnimmer-tri(platform)

@mwoehlke-kitware
Copy link
Contributor Author

We don't need release notes since the buggy PR never made it into any release.

Are you sure? You're probably thinking of #21450, but, while I might be wrong, I think this was introduced by #21146 in early April.

@jwnimmer-tri
Copy link
Collaborator

AIUI for this to crash the build, we need #21450 which is where the assertion happens.

@jwnimmer-tri jwnimmer-tri merged commit 7228fd9 into RobotLocomotion:master Jun 6, 2024
9 checks passed
@mwoehlke-kitware
Copy link
Contributor Author

For it to crash the build (of Drake itself), yes, but I believe any build between April and now has the potential to generate a PEP 440 non-conforming version identifier.

@mwoehlke-kitware mwoehlke-kitware deleted the fix-version-leading-zeroes branch June 6, 2024 17:20
@jwnimmer-tri
Copy link
Collaborator

... has the potential to generate a PEP 440 non-conforming version identifier.

That's actually fine. The leading zero doesn't hurt anyone and is complaint with the spec. The spec says that leading leading zeros shall be ignored: https://packaging.python.org/en/latest/specifications/version-specifiers/#integer-normalization

So really the situation is that we're going beyond the spec and aiming to always emit already-normalized numbers. That's fine, and is probably kinder to downstream code, but (so long as it doesn't crash the build) the leading zero was not a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants