-
Notifications
You must be signed in to change notification settings - Fork 5
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
Proof of concept: Add --pip-platform
flag.
#246
base: master
Are you sure you want to change the base?
Conversation
To pass its value to the `pip install` command, so that it fetches wheels for a specific platform. This will make it possible to install wheels for older operating system versions than the system running pup. For example: `pup ... --pip-platform=macosx_10_12_x86_64`
@click.argument('src') | ||
@command_wrapper | ||
def package(src, ignore_plugins, python_version, launch_module, launch_pyflags, nice_name, icon_path, license_path): | ||
def package(src, ignore_plugins, python_version, launch_module, launch_pyflags, nice_name, icon_path, license_path, pip_platform): |
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.
Hey @carlosperate, picking this up quite a while later... What about having a variation of the launch_pyflags
that could be passed zero/one/more times to have pup
use those flags with pip
? That's basically the behaviour of launch_pyflags
.
Might make the implementation simpler and more versatile at the same time.
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.
Yeah, that's fine with me, feel free to refactor this PR as you see fit 👍
platform_flags.append('--only-binary=:all:') | ||
# TODO: This should probably be done with the spawn helpers, but | ||
# done this way as a proof of concept | ||
site_packages_path = subprocess.check_output([ |
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.
Not sure about this check: what's the objective?
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's not specifically a check, but it is trying to retrieve the "site_packages" path, this path is needed to use the --target
flag, which is needed if we want to use the --platform
flag.
]) | ||
site_packages_path = site_packages_path.decode("utf-8") | ||
if os.path.isdir(site_packages_path): | ||
platform_flags.append('--target') |
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.
In which cases is this needed? AFAICT, pip
is running within the just created Python environment (not virtual, an actual and complete+relocatable install) and will install to the "proper" site-packages
... What am I missing?
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.
We want to use --platform
flag to be able to specify a platform older than the current running system. Specially, for the macOS builds, we might be running this in macOS 12, but want wheels compatible with 10.15, etc.
Unfortunately we cannot use the --platform
flag without adding the --target
flag as well.
--platform
pip flag for older OS compatibility #245This PR is a proof of concept to add a
pip-platform
flag to pup to pass its value to thepip install
command, so that it fetches wheels for a specific platform.This will make it possible to install wheels for older operating system versions than the system running pup.
For example:
pup ... --pip-platform=macosx_10_12_x86_64