-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
More consistent build-binaries.yml
#11478
Conversation
We are not using submodules.
a7720d1
to
62e1d2e
Compare
# pip3 install -U pip | ||
# run: | | ||
# pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall | ||
# pip install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall |
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.
Did you test this change?
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.
No, i only made them for consistency, in case we happen to reactivate it. In that case it would be nice if the re-added code was following the same pattern.
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.
It'd be great to fix #11231 :(
Co-authored-by: Charlie Marsh <[email protected]>
pip install dist/${{ env.PACKAGE_NAME }}-*.whl --force-reinstall | ||
pip install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall |
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.
Why prefer --find-links
over installing the wheel directly?
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.
It works independent of the shell used so we can use it for testing on all platforms
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.
Can't we just request shell: sh
and retain the glob? This feels more complicated.
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.
Currently on main, we have 6x --find-links
and 4x ${{ env.PACKAGE_NAME }}-*
in build-binaries.yml
. I prefer the --find-links
as I can run it locally on any platform and shell, it doesn't rely on shell expansions.
This PR should be ready to merge |
For uv-build, we need to duplicate a lot of the
build-binaries.yml
logic to build another source distribution and wheel. In preparation for that I tried to make the invocations more consistent, to make it easier to review the changes when adding theuv-build
builds on top.Split out from #11446