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

exporter: prepend file for local dependencies #3121

Merged
merged 4 commits into from
Mar 20, 2021

Conversation

maxispeicher
Copy link
Contributor

@maxispeicher maxispeicher commented Oct 7, 2020

Pull Request Check List

When exporting to requirements.txt poetry uses the PEP-508 standard. However pip install supports direct references which need a prependend file:// information for local dependencies. This PR includes this file:// annotation for file and directory depencies.

  • Added tests for changed code.
  • Updated documentation for changed code.

Resolves: #3189

@mrcljx
Copy link

mrcljx commented Oct 7, 2020

Tried to fix the same thing in #3120. Wanted to point out that AFAIK the file:// dependencies have to be absolute:

$ pip install "foo @ file://foo"
ValueError: non-local file URIs are not supported on this platform: 'file://foo'

$ pip install "foo @ file://./foo"
ValueError: non-local file URIs are not supported on this platform: 'file://./foo'

Which means the requirements.txt is not portable and can't be checked into version control.

@mrcljx
Copy link

mrcljx commented Oct 7, 2020

I'm honestly confused why with 1.1 Poetry (apparently deliberately) started to generate pip-incompatible requirements.txt files.

@maxispeicher
Copy link
Contributor Author

maxispeicher commented Oct 7, 2020

Yes actually you're right. It seemed to work for me because the package was already installed and then pip does not try to resolve the path, but as soon as the package should be install in a new environment it does not work. I'll try to fix this.

But yes you're right, its's very confusing. Especially because pip is the only option if you want to be able to specify the target directory.

@abn
Copy link
Member

abn commented Oct 7, 2020

I believe this was implemented according to PEP-440. See python-poetry/poetry-core#22 for implementation. Or is the serialised version incorrect? IIRC, it should only use file:// for absolute paths.

@maxispeicher
Copy link
Contributor Author

Yes the URI notation with file:// should only be used for absolute paths. But I think for generating the requirements.txt file it's fine to transform the relative paths to absolute paths in the URI notation. As we have lock file as a source of truth for the VCS it should not be comitted anyway.
The problem with the current requirements.txt design is that it just is not usable for path dependencies and I think there are two solutions to fix this:

  1. Use absolute paths and use the URI notation with file:// like in this PR
  2. Remove the package name with the @ in front of the package path as in export: fix directory dependencies #3120

@abn
Copy link
Member

abn commented Oct 7, 2020

I'd suggesst that we make it absolute. Since at the point at which we write the serialised version, the Dependnecy instance is not going to be reused, I'd suggest you set the source to absolute, using somehing like this.

if not dependency.source_url.path.is_absolute():
    dependency.source_url = dependency.source_url.path.resolve()

I am not sure if there is a scenario where this will fail for some reason. Would be great to check for nested dependencies with relative path dependencies included.

Prior to the to_pep_508() call. Also, there has been bug fixes in 1.1 for this that have not yet been ported over. So, I might recommend this be rebased against 1.1 now. There are a few issues being fixed in this area.

@abn abn mentioned this pull request Oct 7, 2020
2 tasks
@maxispeicher
Copy link
Contributor Author

maxispeicher commented Oct 7, 2020

I think just using the to_pep_508() function won't work, without changing poetry-core. This is because for file dependencies the URI is generated for absolute paths like:

path = path_to_url(self.path) if self.path.is_absolute() else self.path
requirement += " @ {}".format(path)

however for directory dependencies the logic does not exist

requirement += " @ {}".format(self._path.as_posix())

It's also not possible to transform the directory dependency to a file dependency, because __init__ checks if it is a file or directory.

@maxispeicher
Copy link
Contributor Author

I think for now the best way is to include the logic into exporter.py and making it explicit that the representation diverts from PEP 508.
Additionally I've rebased the branch against the current 1.1 branch and included a simple test for checking if nested path dependencies with relative paths get exported correctly.

@abn abn changed the base branch from master to 1.1 October 7, 2020 23:54
@abn
Copy link
Member

abn commented Oct 8, 2020

You might want to drop the commits not relevant to this PR. I will get back to this later this week.

As for the where the logic lands, I am not too fussed. Just need to make sure the compatibility call to path_to_uri from core is used.

@maxispeicher
Copy link
Contributor Author

I've changed it to a draft PR, as I still want to make some checks and tests regarding some nested dependencies.

@maxispeicher
Copy link
Contributor Author

The exporting seems to work fine with nested dependencies, however the installation seems to cause trouble. As pip creates a tmp folder for executing the build process it cannot resolve relative depdencies from this path. However I'm not sure how to fix this issue, because it would probably need some adaptions in the poetry-core package

@maxispeicher maxispeicher marked this pull request as ready for review October 9, 2020 13:38
@abn abn self-requested a review October 13, 2020 17:14
@abn
Copy link
Member

abn commented Oct 13, 2020

@maxispeicher this is an issue with all scenarios relying on PEP 517 isolated builds and relative path dependenncies. As things stand, the only real way to use them is to use absolute path. This is not something that can resolved in core per say, because the PEP 517 api is only called once the isolated build environment is setup. So unless the package itself specifies that it needs to include the relative path dependencies as part of its sdist this won't work. Then there is the consideration if, poetry, were to be smart about this and lock absolute path, which path should we choose? The one included in the sdist? Or the one that was relative to the project's original source? TLDR; its a slippery slope trying to support nested path dependencies in the lack of any accepted standards. The closest thing we have is PEP 610 - this requires URI.

@maxispeicher
Copy link
Contributor Author

@abn Do you think you can manage to review this one, as it is still a blocker from some projects.

@AndrewGuenther
Copy link

I recently ran into this issue. Any way I can help get this landed? Maybe just a friendly nudge to @abn ?

Copy link

github-actions bot commented Mar 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants