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.

Sign up for GitHub

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

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

Proof of concept: Add --pip-platform flag. #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/pup/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ def main(log_level):
@click.option('--nice-name')
@click.option('--icon-path')
@click.option('--license-path')
@click.option('--pip-platform', default=None)
@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):
Copy link
Member

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.

Copy link
Member Author

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 👍

"""
Packages the GUI application in the given pip-installable source.
"""
Expand All @@ -88,4 +89,5 @@ def package(src, ignore_plugins, python_version, launch_module, launch_pyflags,
nice_name=nice_name,
icon_path=icon_path,
license_path=license_path,
pip_platform=pip_platform,
)
5 changes: 4 additions & 1 deletion src/pup/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def _context(
nice_name=None,
icon_path=None,
license_path=None,
pip_platform=None,
):

return context.Context(
Expand All @@ -35,6 +36,7 @@ def _context(
license_path=license_path,
ignore_plugins=ignore_plugins,
platform=sys.platform,
pip_platform=pip_platform,
python_version=python_version,
)

Expand All @@ -50,11 +52,12 @@ def package(
nice_name=None,
icon_path=None,
license_path=None,
pip_platform=None,
):

_log.info('Package %r: starting.', src)

ctx = _context(ignore_plugins, src, python_version, launch_module, launch_pyflags, nice_name, icon_path, license_path)
ctx = _context(ignore_plugins, src, python_version, launch_module, launch_pyflags, nice_name, icon_path, license_path, pip_platform)
dsp = dispatcher.Dispatcher(ctx)

dsp.collect_src_metadata()
Expand Down
3 changes: 3 additions & 0 deletions src/pup/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ def __init__(
license_path,
ignore_plugins,
platform,
pip_platform,
python_version,
):

Expand Down Expand Up @@ -52,6 +53,8 @@ def __init__(

self.final_artifact = None

self.pip_platform = pip_platform


@property
def nice_name(self):
Expand Down
23 changes: 22 additions & 1 deletion src/pup/plugins/pip_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import logging
import subprocess
import os


_log = logging.getLogger(__name__)
Expand All @@ -25,13 +26,33 @@ def usable_in(ctx):
)

def __call__(self, ctx, dsp):
python_path = str(ctx.python_runtime_dir / ctx.python_rel_exe)

platform_flags = []
if ctx.pip_platform:
platform_flags.append('--platform={}'.format(ctx.pip_platform))
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([
Copy link
Member

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?

Copy link
Member Author

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.

python_path,
'-c',
'import site; print(site.getsitepackages()[0], end="")'
])
site_packages_path = site_packages_path.decode("utf-8")
if os.path.isdir(site_packages_path):
platform_flags.append('--target')
Copy link
Member

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?

Copy link
Member Author

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_flags.append('{}'.format(site_packages_path))
else:
raise Exception("Invalid site-packages directory: {}".format(site_packages_path))

cmd = [
str(ctx.python_runtime_dir / ctx.python_rel_exe),
python_path,
'-m',
'pip',
'install',
'--no-warn-script-location',
*platform_flags,
ctx.src_wheel,
]

Expand Down