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

find_ruff_bin does not work as a build dependency with pip #13321

Closed
gaborbernat opened this issue Sep 10, 2024 · 19 comments · Fixed by #13591
Closed

find_ruff_bin does not work as a build dependency with pip #13321

gaborbernat opened this issue Sep 10, 2024 · 19 comments · Fixed by #13591
Labels
question Asking for support or clarification

Comments

@gaborbernat
Copy link
Contributor

gaborbernat commented Sep 10, 2024

See reproducible at https://github.com/gaborbernat/ruff-find-bin-during-pip-build/actions/runs/10802198412/job/29963805989

With code gaborbernat/ruff-find-bin-during-pip-build@fd532e8

In this case, the host Python is the installer Python, but the expectation is to find it in the pip isolated environment

cc @pradyunsg I am not sure if this is a pip bug or a uv bug... but the binary gets discovered correctly when installing with uv, but discovers in host Python when using pip.

cc @charliermarsh I believe this is also a bug for uv itself for find_uv_bin...

@zanieb
Copy link
Member

zanieb commented Sep 11, 2024

This doesn't seem like a Ruff bug? It looks like pip isn't installing the build requirement (ruff) before calling the hook. In uv, we install the build requirement into the isolated build environment. I'm not sure what pip is doing here or if it's intentional. I also tried enabling PEP 517 with the same result:

zanieb/ruff-find-bin-during-pip-build@2cd719e
https://github.com/zanieb/ruff-find-bin-during-pip-build/actions/runs/10819384824/job/30017135074

@zanieb zanieb added the question Asking for support or clarification label Sep 11, 2024
@gaborbernat
Copy link
Contributor Author

gaborbernat commented Sep 11, 2024

I do not believe what you say is correct. If you check the logs, you can see that Pip actually is installing the package:

Processing /home/runner/work/ruff-find-bin-during-pip-build/ruff-find-bin-during-pip-build
Installing build dependencies: started
Installing build dependencies: finished with status 'done'

And actually the stack trace confirms this:

         File "/home/runner/work/ruff-find-bin-during-pip-build/ruff-find-bin-during-pip-build/build.py", line 7, in prepare_metadata_for_build_wheel
          raise RuntimeError(find_ruff_bin())
                             ^^^^^^^^^^^^^^^
        File "/tmp/pip-build-env-dkih1szy/overlay/lib/python3.12/site-packages/ruff/__main__.py", line 36, in find_ruff_bin
          raise FileNotFoundError(scripts_path)

See ruff is installed into /tmp/pip-build-env-dkih1szy/overlay/.

The problem is that Pip does not use virtual environments for building packages, but rather isolated environments. And the discovery mechanism here assumes this is going to be a virtual environment.

I will leave it to you to argue with the PIP maintainers if PEP 517 requires a virtual environment or an isolated environment should suffice. Actually, the PEP explicitly calls it out that virtual environments not required: https://peps.python.org/pep-0517/#build-environment

We do not require that any particular “virtual environment” mechanism be used; a build frontend might use virtualenv, or venv, or no special mechanism at all.

Which then makes it a ruff bug. find_ruff_bin must handle pips isolated environment.

You enabling PEP-517 is a no op because PEP-517 is enabled by default in pip.

@zanieb
Copy link
Member

zanieb commented Sep 12, 2024

What is the isolated environment you refer to? How are they creating it? Does it follow a specification somewhere? Can its root be discovered using sysconfig?

You enabling PEP-517 is a no op because PEP-517 is enabled by default in pip.

Is this true? I think it's enabled by default sometimes, but not always? We get issues every day of people reporting different uv vs pip behavior because pip isn't using build isolation on their system.

Just wondering, why are you using Ruff as a build dependency?

@charliermarsh
Copy link
Member

You enabling PEP-517 is a no op because PEP-517 is enabled by default in pip.

(In general, I don't believe this is true. It's enabled by default for pyproject.toml-based projects. It is not enabled by default for projects that only contain a setup.py. See pypa/pip#9175. But this is a pyproject.toml, so yes, shouldn't be affected by the flag.)

@gaborbernat
Copy link
Contributor Author

gaborbernat commented Sep 12, 2024

Is this true? I think it's enabled by default sometimes, but not always? We get issues every day of people reporting different uv vs pip behavior because pip isn't using build isolation on their system.

I think for all modern packages. Which is the case here too.

Just wondering, why are you using Ruff as a build dependency?

Have an app that generates during build some code from a JSON schema, and the library uses ruff afterward to format the generated code.

What is the isolated environment you refer to?

From the above stack trace:

/tmp/pip-build-env-dkih1szy/overlay/

How are they creating it? Does it follow a specification somewhere? Can its root be discovered using sysconfig?

A pip maintainer would be able to answer these questions better than me. Note, that PEP-517 never requires the isolated environment to be sysconfig discoverable and I assume is not.

@gaborbernat
Copy link
Contributor Author

https://github.com/pypa/pip/blob/main/src/pip/_internal/build_env.py#L81-L139 I believe contains the implementation detail answers... perhaps a cheap solution would be to look for ruff binary on the last entry of PATH... FWIW.

@zanieb
Copy link
Member

zanieb commented Sep 12, 2024

Thanks for pulling that up. I think we've explicitly avoided finding the Ruff binary on the PATH because we can't really know that it's the correct Ruff executable associated with the module. I'm looking at the site code trying to figure out if there's a clear way to guarantee that.

@gaborbernat
Copy link
Contributor Author

gaborbernat commented Sep 12, 2024

FWIW using uv over pip fixes the issue, but some of our internal systems are not uv ready yet...

@zanieb
Copy link
Member

zanieb commented Sep 12, 2024

I guess I'd just recommend using a different strategy to find the Ruff binary in the meantime, like shutil.which — since you know in this situation it will be at the front of the path. I'm not sure this is an intended use-case for find_ruff_bin. If there was an obvious fix that retained the existing semantics I'd accept it though.

@gaborbernat
Copy link
Contributor Author

I don't control the package doing the generation and this method is name for solving this problem, needing another solution doesn't feels right...

@pradyunsg
Copy link

Yea, pip's isolation is a weird setup that's not quite a virtual environment for $legacy reasons around how things worked with Python 2 and all that IIRC. I've wanted to clean that up for a while but haven't come around to it due to a lack of time.

I don't think you can get the information about the isolated environment's paths from sysconfig.

@gaborbernat
Copy link
Contributor Author

So @zanieb what is the action plan here?

@gaborbernat
Copy link
Contributor Author

@zanieb ping 😊

@zanieb
Copy link
Member

zanieb commented Sep 24, 2024

Please don't ping us repeatedly — it's bad form.

I don't have an action plan here — it's a weird use case and there's not a clear solution. The upstream package should probably switch from using find_ruff_bin to read the PATH directly.

If there's a heuristic we can use to understand we're in a bespoke pip build environment, I'm all ears.

@gaborbernat
Copy link
Contributor Author

How about this close-enough approximation:

  • when all other discovery fails,
  • if the first entry on the PATH contains a folder named pip-build-env- (starting with) and within overlay
  • a ruff executable is present on the first entry of PATH

We take a ruff from the first entry of PATH. If you agree with this I can create a PR.

If we really want to be safe, we can also check the presence of the overlay and normal entries at the end of sys.path.

Thanks!

@zanieb
Copy link
Member

zanieb commented Sep 26, 2024

It seems reasonable from a correctness perspective, but a bit dubious for long-term maintenance. I'm not sure. Let me see what other people on the team think.

@gaborbernat
Copy link
Contributor Author

gaborbernat commented Sep 26, 2024

FWIW pip at some point will switch over to using virtual environments, at which point we can remove it. (or pip will die and be replaced by uv at which point again we can remove it). So either way is a temporary heuristic to make it work in the current (inperfect) world.

@charliermarsh
Copy link
Member

I suppose I'm alright with it. (Is that a reliable heuristic on all platforms?)

@gaborbernat
Copy link
Contributor Author

Yes, following their implementation of the isolated build environment, this should work reliably on all platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants