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

Replace subpar with a cross-platform solution #436

Closed
UebelAndre opened this issue Mar 19, 2021 · 17 comments
Closed

Replace subpar with a cross-platform solution #436

UebelAndre opened this issue Mar 19, 2021 · 17 comments

Comments

@UebelAndre
Copy link
Contributor

UebelAndre commented Mar 19, 2021

🚀 feature request

Description

A clear and concise description of the problem or missing capability...

I opened this as a feature request because I otherwise didn't know how to raise open the issue

Right now the rules use subpar to generate some executable which are then committed back and reused for other rules:

par_binary(
name = "piptool",
srcs = ["piptool.py"],
deps = [
":whl",
requirement("pip"),
requirement("wheel"),
],
)
par_binary(
name = "whltool",
srcs = ["whl.py"],
main = "whl.py",
deps = [
":whl",
],
)

Is there a reason these couldn't be converted to using Bazel's python_zip_file functionality to accomplish the same thing? (introduced in bazelbuild/bazel#9453)

@UebelAndre
Copy link
Contributor Author

If this is possible, would it solve for the outstanding issue on #344

@UebelAndre
Copy link
Contributor Author

is the generated zip (zipapp? PEP-0441) cross platform? Does anyone know it's limitations there? I'm pretty surprised this is not a more advertised feature.

@hrfuller
Copy link
Contributor

hrfuller commented Mar 22, 2021

The .zip won't be cross-platform. Any rules_python pip dependencies are fetched only for the local platform. In some cases with universal wheels they could be cross-platform, but it isn't a guarantee. Having said that and without in depth knowledge I don't think subpar's are truly cross-platform either.

@UebelAndre
Copy link
Contributor Author

Is there currently a cross platform implementation for self contained Python apps?

@bshashank
Copy link

The .zip won't be cross-platform. Any rules_python pip dependencies are fetched only for the local platform. In some cases with universal wheels they could be cross-platform, but it isn't a guarantee. Having said that and without in depth knowledge I don't think subpar's are truly cross-platform either.

I agree, subpar and --build_python_zip behave in a similar manner wrt cross-platform. The one advantage of subpar is that you can ask for the par to be unzipped before execution - which helps Python discover compiled code (.so or .dlls) more easily.

Since pip and wheel are both pure-python, I think both subpar and --build_python_zip would work in the same manner.

@UebelAndre
Copy link
Contributor Author

Except in that the zips are producible on windows and the pars are not, right?

@hrfuller
Copy link
Contributor

FWIW if cross-platform support is a necessity, which it may not be here, https://github.com/pantsbuild/pex has cross-platform support.

@UebelAndre
Copy link
Contributor Author

Nice! Are there Bazel rules for that?

Also, do all Python rules currently work on all platforms? I thought the par files were a bottleneck for windows

@shridevinb
Copy link

I need to build .par files on windows. is there a workaround for it apart from using --build_python_zip?

@UebelAndre
Copy link
Contributor Author

For what it's worth, I found @apache_incubator_heron//tools/rules/pex which seems to be a fork of https://github.com/benley/bazel_rules_pex at first glance but perhaps those rules can be used to finally address #2 ?

@UebelAndre UebelAndre changed the title Replace subpar with build_python_zip Replace subpar with a cross-platform solution Jun 26, 2021
@UebelAndre
Copy link
Contributor Author

I also came across https://github.com/arrdem/rules_zapp which seems like it might solve for some of the needs here.

@groodt
Copy link
Collaborator

groodt commented Sep 2, 2021

The two .par files in the repo are only used to support the legacy pip_import mechanism as far as I recall.

Maybe @brandjon or other maintainers can confirm if this can be dropped in the next release and then the need for the subpar dependency in these rules is removed.

If not, the .par files used as part of rules_python only take a dependency on pure Python wheels (pip, wheel, setuptools).

See here.

The .par files could be replaced by --build_python_zip. No additional dependencies would be required. It would work cross-platform because these dependencies are pure Python. Note: it would NOT make all the rules automatically cross-platform. It would only make the rules "execute" on Windows, which is a step forward I guess.

If I recall correctly, the shebang in a zip from --build_python_zip is #!/usr/bin/env python, which is supported on POSIX and also the Windows Python launcher. Of course, be careful of Python2 vs Python 3 on the PATH (but you don't use python2 anymore right? ... right?)

This does of course require Python to be installed on the PATH of the host and is not hermetic / toolchain aware, but this is true of the current situation with subpar as well as far as I understand.

This is why I think the easiest path forward is to remove the legacy pip_import mechanism.

@alexeagle
Copy link
Collaborator

@thundergolfer @hrfuller feels to me like we are nearly two years past EOL for Python 2 and dropping some tech debt from rules_python would benefit us, so I'd like to adopt @groodt proposal here to remove the legacy pip_import along with anything which exists only to support it. Losing all .par files and the machinery to produce them seems totally worth the tradeoff for users who will get stuck on an older rules_python.
WDYT?

@UebelAndre
Copy link
Contributor Author

If it helps any, dropping the legacy code would make investigations and feature development easier. I've gotten myself confused as to why I don't see the behavior or code I expect only to find i'm looking at the wrong file but didn't notice because it's so similar.

@hrfuller
Copy link
Contributor

@alexeagle I agree its time to ditch them. We should work on some rudimentary pex rules to replace par in the future.

@UebelAndre
Copy link
Contributor Author

Since the use of subpar was removed in #582, there's no longer a need for a cross-platform solution. Closing this issue out as a result.

@alexeagle
Copy link
Collaborator

Thanks Andre!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants