-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Conversation
+@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. |
There was a problem hiding this 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.)
There was a problem hiding this 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.
Previously, jwnimmer-tri (Jeremy Nimmer) 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 |
54922b3
to
fa3f963
Compare
There was a problem hiding this 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 fiNo 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_SHAAnd 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Blocked on #21968.
This change is