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

[build] Publish linux wheels for Python 3.13 #22580

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

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Feb 2, 2025

Blocked on #21968.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added status: do not merge priority: medium release notes: fix This pull request contains fixes (no new features) labels Feb 2, 2025
@jwnimmer-tri
Copy link
Collaborator Author

+@mwoehlke-kitware Low priority, but when you have some time, please take a look as feature reviewer.

I believe all of the changes here are finished / correct / cohesive, but as noted in the overview this is blocked on #21968 -- so if we tried to run the wheel build on this PR, it would halt with pybind11 compilation errors.

If you're super curious you can rebase this atop https://github.com/jwnimmer-tri/drake/commits/pybind11-tmp/ which is the WIP for #21968, and then it does successfully create a wheel.

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)


tools/wheel/image/build-python.sh line 26 at r1 (raw file):

wget $URL
sha256sum "$ARCHIVE"

Was this intentional? (If "yes", it's inefficient and of questionable usefulness.)

Copy link
Collaborator Author

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)


tools/wheel/image/build-python.sh line 26 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…

Was this intentional? (If "yes", it's inefficient and of questionable usefulness.)

Yes. Any time a checksum fails, the script should print out the empirical checksum to aid the developer, e.g., when adding a new download as we have here. I shouldn't need to go figure out what it was trying to download when I add a new version number.

I could add some "echo...." message in front of the printout to provide context that makes that more clear?

In a build that takes half an hour, I am not sure the 100 milliseconds wasted here is inefficient enough to merit additional code complexity to only print when in case it failed.

@mwoehlke-kitware
Copy link
Contributor

tools/wheel/image/build-python.sh line 26 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes. Any time a checksum fails, the script should print out the empirical checksum to aid the developer, e.g., when adding a new download as we have here. I shouldn't need to go figure out what it was trying to download when I add a new version number.

I could add some "echo...." message in front of the printout to provide context that makes that more clear?

In a build that takes half an hour, I am not sure the 100 milliseconds wasted here is inefficient enough to merit additional code complexity to only print when in case it failed.

# Change SHA elsewhere to EXPECTED_SHA
readonly ACTUAL_SHA=$(sha256sum $ARCHIVE | cut -d' ' -f1)
if [[ $ACTUAL_SHA != $EXPECTED_SHA ]]; then
  echo ... >&2
  exit 1
fi

No gratuitous inefficiency, better message in case of failure. (Writing said better message left as an exercise to the PR author.)

Alternatively, if you're that concerned about "additional code complexity to only print when in case it failed":

readonly ACTUAL_SHA=$(sha256sum $ARCHIVE | cut -d' ' -f1)
echo "  Expected SHA: $EXPECTED_SHA"
echo "    Actual SHA: $ACTUAL_SHA"
test $ACTUAL_SHA == $EXPECTED_SHA

And in any case, I encourage keeping the blank line after wget. 🙂

Copy link
Collaborator Author

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee mwoehlke-kitware, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)


tools/wheel/image/build-python.sh line 26 at r1 (raw file):

Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
# Change SHA elsewhere to EXPECTED_SHA
readonly ACTUAL_SHA=$(sha256sum $ARCHIVE | cut -d' ' -f1)
if [[ $ACTUAL_SHA != $EXPECTED_SHA ]]; then
  echo ... >&2
  exit 1
fi

No gratuitous inefficiency, better message in case of failure. (Writing said better message left as an exercise to the PR author.)

Alternatively, if you're that concerned about "additional code complexity to only print when in case it failed":

readonly ACTUAL_SHA=$(sha256sum $ARCHIVE | cut -d' ' -f1)
echo "  Expected SHA: $EXPECTED_SHA"
echo "    Actual SHA: $ACTUAL_SHA"
test $ACTUAL_SHA == $EXPECTED_SHA

And in any case, I encourage keeping the blank line after wget. 🙂

Done.

I guess I was avoiding moving the pass/fail to bash, but on reflection it seems fine.

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features) status: do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants