search

RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.17k stars 1.24k forks source link

Strip leading zeroes from version #21532

Closed mwoehlke-kitware closed 3 weeks ago

mwoehlke-kitware commented 3 weeks ago

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

mwoehlke-kitware commented 3 weeks ago

+@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.

mwoehlke-kitware commented 3 weeks ago

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 commented 3 weeks ago

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

mwoehlke-kitware commented 3 weeks ago

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.

jwnimmer-tri commented 3 weeks ago

... 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.