Skip to content

Conversation

@BrandonLWhite
Copy link
Contributor

@BrandonLWhite BrandonLWhite commented Oct 25, 2024

The motivation for this PR is to add a new flag --platform that achieves the same effect as the pip option of the same name which is to facilitate the installation of wheels that are compatible with the platform specified.

The primary use case is in CI/CD operations to produce a deployable asset, such as a ZIP file for AWS Lambda and other such cloud providers. It is common for the runtimes of these target environments to be different enough from the CI/CD's runner host such that the binary wheels selected using the host's criteria are not compatible with the target system's.

Notes:

  • This is not an actual cross-compiler. It simply allows controlling which prebuilt binaries are selected.
  • I am committed to adding further documentation to the README for this setting if the project maintainers are open to accepting this feature.
  • The env._supported_tags assignment that you will see in this code is not ideal, and I welcome any input for a better way. Since the Poetry code does not provide a public method for mutating this value, I resorted to mutating this pseudo-private field.
  • I tried to use as much as I could from the packaging module, but I did have to reinvent the wheel for some things due to that module being hard-coded to call out to the host system to query certain values, which we are specifically trying to avoid here. In my research, I noted that the pip code effectively does the same thing. Should such a reusable module exist for producing the Tags combinations that are compatible with the specified platform, it would be much better to use that. I was not successful in finding such a library.

@radoering
Copy link
Member

I think I am open to this feature. Even if parts of the implementation feel a bit hacky, the good thing is that it is well encapsulated and will not influence users who do not use the feature. Maybe, we should call the feature "experimental" in its description and the readme?

This is not an actual cross-compiler. It simply allows controlling which prebuilt binaries are selected.

What will happen for sdist only releases with extensions? Will installation fail? WIll the extension be built for the wrong platform and installed without an error so that you only will notice that your environment is broken when using it?

I am committed to adding further documentation to the README for this setting if the project maintainers are open to accepting this feature.

Sounds good.

The env._supported_tags assignment that you will see in this code is not ideal, and I welcome any input for a better way. Since the Poetry code does not provide a public method for mutating this value, I resorted to mutating this pseudo-private field.

At first, I thought introducing a public method does not make sense and it would be better to have a subclass of VirtualEnv that allows changing the tags. However, this probably does not lead to a better solution because it is not easy to replace VirtualEnv with the subclass. On the other side, if we add a public method, ideally with a comment that this is used by the bundle plugin, the risk of breaking the interface is less likely. In the long term, that probably makes sense. For now, I think it is ok to keep it as is.

In my research, I noted that the pip code effectively does the same thing. Should such a reusable module exist for producing the Tags combinations that are compatible with the specified platform, it would be much better to use that. I was not successful in finding such a library.

I do not know of any such library either. Some explicit unit tests for the platform module would be nice.

@BrandonLWhite
Copy link
Contributor Author

Thanks for the response. I will investigate the questions and report back. I'll also include some utils/platforms.py specific tests beyond the integration coverage in test_venv_bndler

@BrandonLWhite
Copy link
Contributor Author

BrandonLWhite commented Jan 14, 2025

@radoering, this is ready for your (re)review! I believe I've addressed all of your prior feedback comments. Please let me know if you need more or want to see something different.

Regarding your questions:

What will happen for sdist only releases with extensions? Will installation fail? WIll the extension be built for the wrong platform and installed without an error so that you only will notice that your environment is broken when using it?

I tried to elaborate on this in the README. See what you think. But to answer your questions directly:

Will installation fail?

Yes, if the host system doesn't have the prerequisites to build it. Otherwise, no, it will build it like "normal" and that output is what is installed.

WIll the extension be built for the wrong platform and installed without an error so that you only will notice that your environment is broken when using it?

Yes, assuming the build succeeds and the host system is the wrong platform. The --platform option here has no effect.

@BrandonLWhite
Copy link
Contributor Author

Hi @radoering. Could you please approve or advise on any further changes you require?

@radoering
Copy link
Member

@BrandonLWhite Sorry for the long response time. The past few weeks, we focussed on Poetry 2.1 - and I assume we will have to focus on regressions for the next 1-2 weeks. I will try to review this one afterwards.

@mrbusche
Copy link

mrbusche commented Mar 6, 2025

One data point - We have been using this branch/commit to pip install for a few weeks now and it has been working great for us.

@radoering radoering merged commit cfec650 into python-poetry:main Mar 9, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants