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

hooks.get_requires_for_build_wheel may return pep 508 url, but BuildEnvironment.pip_install() cannot process that #43

Closed
ghost opened this issue Feb 7, 2019 · 15 comments
Labels

Comments

@ghost
Copy link

ghost commented Feb 7, 2019

The hooks.get_requires_for_build_wheel function may return a pep 508 url if that is used in install_requires() (which works completely fine outside of pep517 usage, I can install this package just fine), but BuildEnvironment.pip_install() cannot process that. However, this is exactly how envbuild.py uses things:

...
with BuildEnvironment() as env:
        env.pip_install(requires)
        reqs = hooks.get_requires_for_build_wheel(config_settings)
        env.pip_install(reqs)

And this will lead to the following error:

Requirement 'e @ https://github.com/e/e/archive/master.zip' looks like a filename, but the file does not exist
Processing ./e @ https:/github.com/e/e/archive/master.zip
Could not install packages due to an EnvironmentError: [Errno 2] No such file or directory: '/home/e/Develop/python-for-android/e @ https:/github.com/e/e/archive/master.zip'

So it looks to me like either envbuild.py or pip are broken here

@ghost
Copy link
Author

ghost commented Feb 20, 2019

Could someone give me just a very extremely basic feedback whether this is a pip error, pep517 bug, or something else? Because I run into the same issue outside of pep517 in my own handling somewhere else and I got a workaround I'm not particularly fond of, so I'm interested in finding out with whom the culprit lies here so I can find out how to properly address this in my own code

@pfmoore
Copy link
Member

pfmoore commented Feb 20, 2019

From here, "This hook MUST return an additional list of strings containing PEP 508 dependency specifications,"

wobblui @ https://github.com/JonasT/wobblui/archive/master.zip is a PEP 508 URL requirement.

However, the pip manual points out that "pip does not support the url_req form of specifier at this time" - things are currently moving fast in this area, so I think it would be best to assume "limited support" for URL specs, at best.

@ghost
Copy link
Author

ghost commented Feb 20, 2019

But the PEP 508 URL format works in both setuptool's setup_requires and install_requires, even when installing the package with pip. This leads to the weird situation that this package is perfectly able to be installed including with pip, but this line will always fail because setup_requires of this package contains such a PEP 508 URL and it appears to be passed to pip as-is:

https://github.com/pypa/pep517/blob/970f48beebf29b0831a3a067da4a0a3daac20193/pep517/envbuild.py#L135

Isn't this something that should be fixed timely? Surely the envbuild.py code is supposed to be able to handle any package that would also install properly through pip?

Edit: minor clarifications
Edit2: linking actual line

@ghost
Copy link
Author

ghost commented Feb 20, 2019

In fact, now that I recall why I used it, I think setuptools didn't like the #egg-name=... URLs pip prefers in its requirements.txt as external repository sources and I actually had to use PEP 508 URLs to get the dependency install to work without weird extra flags (like --process-dependency-links).

So basically now I can choose whether the package actually installs properly with setuptools & regular pip use, vs whether it works with pep517 and envbuild.py. That seems quite unfortunate

@ghost
Copy link
Author

ghost commented Mar 1, 2019

@pfmoore any ideas on how to move forward? are packages that use PEP 508 URLs in setup_requires meant to break in pep517's envbuild? (even though they install fine outside of it) if not, maybe there could be some coordination with pip guys, I made a ticket over there: pypa/pip#6306

@pradyunsg
Copy link
Member

@Jonast, @pfmoore is a pip maintainer. :)

@ghost
Copy link
Author

ghost commented Mar 2, 2019

@pradyunsg ah neat 😄 just to emphasize this, I'd be happy not to use PEP 508 URLs at all, but as described above I found no other way to make my package's deps work, and it does work outside of pep517

@cjerdonek
Copy link
Member

@Jonast I believe this is the main issue on pip's tracker for discussing the overall issue and workarounds: pypa/pip#5898
(@pradyunsg, feel free to correct me if I'm wrong.)

@pfmoore
Copy link
Member

pfmoore commented Mar 2, 2019

@pfmoore any ideas on how to move forward?

Probably someone needs to write a PR addressing pypa/pip#6306. Personally, I don't use either setup_requires or URL links, so it won't be me doing that ;-)

Also, I'd point out that pip doesn't use envbuild from this project (it has its own environment builder) so this isn't a high priority issue for pip. (In fact, I know of no-one other than you using the envbuild module). While it's quite possible that this issue also affects pip's environment builder, we've had no other issues raised about it, so it's likely extremely rare usage.

@ghost
Copy link
Author

ghost commented Mar 2, 2019

@Jonast I believe this is the main issue on pip's tracke

@cjerdonek that's a different issue, that's "PEP 508 URLs don't support versions / are inadequate when they work". My issue is that they actually don't even work in some places, e.g. pep517's envbuild or pip install -r (which is related)

While it's quite possible that this issue also affects pip's environment builder, we've had no other issues raised about it

Um well, didn't I just do exactly that? 😆 This DOES definitely break my package. Edit 2: oops, misread, no idea about pip's environment builder I just used pep517's envbuild

Edit: related note: I am btw interested in contributing highlevel dependency analysis code to PEP517 which I think would be a great addition, but I can't finish it purely because of this issue. (It forced me to have an ugly, likely incorrect-in-corner-cases workaround, and I can't remove it because it breaks my package unless this is fixed/addressed somehow)

@ghost
Copy link
Author

ghost commented Mar 2, 2019

Basically I have a package that depends on e @ https://github.com/e/e/archive/master.zip at setup time (setup_requires) - this breaks envbuild. The package itself isn't published yet because I'm still working on it, but that doesn't make the problem any less real

@pfmoore
Copy link
Member

pfmoore commented Mar 2, 2019

@Jonast Sorry, I may have misunderstood your pip issue, which says "any setuptools package that will have them in setup_requires will result in pep517's envbuild.py attempting to install it with pip". I assumed from that, you were referring to this project's envbuild.py. Calling this project pep517 may have been a confusing choice ;-)

Anyway, my point remains. Someone needs to submit a PR.

@ghost
Copy link
Author

ghost commented Mar 2, 2019

Yeah I meant this project's envbuild.py, I was just being silly and misread your comment 🤷‍♀️ never used pip's environment builder

@pradyunsg pradyunsg added the bug label Aug 27, 2020
@pradyunsg
Copy link
Member

pradyunsg commented Aug 27, 2020

To reiterate/summarize, pypa/pip#5898 is the underlying issue here, and we'd be willing to accept a PR fixing that or working around this here. :)

@takluyver
Copy link
Member

I see some hints that something relevant may have been fixed in pip, e.g. pypa/pip#6203

We've come to an agreement that the functionality to create isolated build environments and install build dependencies will live in the PyPA build project. The pep517 library will continue to exist as the lower-level piece to work with the interfaces defined by PEP 517, but the pep517.envbuild module and the little command-line interfaces which use it (pep517.build, pep517.check and pep517.meta) will gradually be deprecated and eventually removed as build becomes more stable. See #91 for more on that process.

So I'm closing this, as there won't be any further significant work here on BuildEnvironment. And it doesn't make sense to fix it at a lower level, because the URL requirement should be valid.

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

No branches or pull requests

4 participants