poetry-core
poetry-core copied to clipboard
python-poetry
Fix bug/crash when source dir is outside project dir
Hello poetry team,
here is a draft fix for issue python-poetry#6521. It does not currently include a test, as I am not sure how to write a test for it. I tried perusing the unit tests in tests/masonry/builders/test_builder.py and got a bit lost :sweat_smile:
Any help or guidance on unit tests would be much appreciated
Documentation... I'm not sure if that's necessary? The purpose of this change is to make poetry do the right thing, instead of failing unexpectedly... I will defer to your judgement on that.
replace usage of pathlib.Path.relative_to with os.path.relpath, as per https://github.com/python-poetry/poetry/issues/5621
Resolves: python-poetry#6521
- [ ] Added tests for changed code.
- [ ] Updated documentation for changed code.
Don't think this is a good idea. Since this will fail isolated builds.
python -m pip build
python -m build .
python -m pip install --use-pep517 .
Don't think this is a good idea. Since this will fail isolated builds.
python -m pip build python -m build . python -m pip install --use-pep517 .
I'm not sure I follow...
my pip installation (v22.0.4) does not have a build command and my virtualenv (python 3.10) does not have a package / module called 'build', so i can't test those first two commands, but python -m pip install --use-pep517 . works exactly as intended.
Can you please elaborate?
Oops. I meant python -m pip install build :)
On reading up a bit, I am inclined to say that we should favour an in-place build in general. This means your change is in theory okay. The current error is expected because we needed to ensure that the files being included are "sub-paths". This was prior to pip making in-tree builds default (https://github.com/pypa/pip/pull/10495). This meant that the project directory would get copied over into a temporary directory and then the build backend would be called. This would cause an error as the directory copied would not contain the source being included in your example. Also on checking the code for build, seems it does not really care where the source is so poetry-core can make the decision.
In pip >=21.3,<22.1b1 you can reproduce the issue (i think) via
pip wheel --no-deps --use-deprecated=out-of-tree-build .
However, this also lends itself for users to shoot themselves in the foot when their project gets built with build isolation or if folks start including packages outside git root for example. Additionally, there may be inconsistencies if a PEP 517 frontend is used that uses build isolation. I suspect this is an okay trade-off.
All this, is to say that we should probably avoid the relative path check altogether, but rather check if the file/directory actually exist.
As for testing I would recommend adding a new fixture in https://github.com/python-poetry/poetry-core/tree/master/tests/fixtures and that includes sources from ../<another fixture> and run it through a complete build, sdist build, and wheel build. You can find specific example cases you can derrive from at https://github.com/python-poetry/poetry-core/tree/master/tests/masonry/builders.
Ahh, ok, that makes a lot of sense. Thanks
I'm writing unit tests now, and testing has revealved something interesting: when building a porject with a source root outside the project root, editable installs and wheels build correctly, but sdists do not. 😞
You wind up with source files in the tar file like <project_name>/../library/__init__.py, which tar fails to unpack
hmmm....
hmmm.... I've been poking at this with a debugger, and it seems like when poetry is packaging sdist archives, it generates a list of BuildIncludeFiles whose source root attribute is not set to the actual source root. when this happens, those files' relative paths are calculated relative to the project root.
I suspect this line is the culprit: https://github.com/python-poetry/poetry-core/blob/0904607b405185a37acb34e77f6d2fe597927e8e/src/poetry/core/masonry/builders/builder.py#L177
What is the purpose of that check? I can clearly see that it's important, it was added for a reason, I just don't understand what problem it solves
What is the purpose of that check? I can clearly see that it's important, it was added for a reason, I just don't understand what problem it solves
This is a bit unclear in the code at present. However, the reason why it is there right now is because the file layout in the sdist archive mirrors what is the actual source layot whereas in wheels you want it in a layout that is similar to how it gets exploded into site-packages.
For example if you take poetry-core itself, you would want the tarball to contain the src directory as is, whereas in the wheel we do not need it. See below. I have removed vendor packages and test files for readability.
Tarball:
tarball/
└── poetry-core-1.1.0b1
├── LICENSE
├── PKG-INFO
├── pyproject.toml
├── README.md
├── src
│ └── poetry
│ └── core
│ ├── exceptions
│ │ ├── base.py
│ │ └── __init__.py
│ ├── factory.py
│ ├── __init__.py
│ ├── json
│ │ ├── __init__.py
│ │ └── schemas
│ │ └── poetry-schema.json
│ ├── lock
│ ├── masonry
│ │ ├── api.py
│ │ ├── builder.py
│ │ ├── builders
│ │ │ ├── builder.py
│ │ │ ├── __init__.py
│ │ │ ├── sdist.py
│ │ │ └── wheel.py
│ │ ├── __init__.py
│ │ ├── metadata.py
│ │ └── utils
│ │ ├── helpers.py
│ │ ├── include.py
│ │ ├── __init__.py
│ │ ├── module.py
│ │ └── package_include.py
│ ├── packages
│ │ ├── constraints
│ │ │ ├── any_constraint.py
│ │ │ ├── base_constraint.py
│ │ │ ├── constraint.py
│ │ │ ├── empty_constraint.py
│ │ │ ├── __init__.py
│ │ │ ├── multi_constraint.py
│ │ │ └── union_constraint.py
│ │ ├── dependency_group.py
│ │ ├── dependency.py
│ │ ├── directory_dependency.py
│ │ ├── file_dependency.py
│ │ ├── __init__.py
│ │ ├── package.py
│ │ ├── project_package.py
│ │ ├── specification.py
│ │ ├── url_dependency.py
│ │ ├── utils
│ │ │ ├── __init__.py
│ │ │ ├── link.py
│ │ │ └── utils.py
│ │ └── vcs_dependency.py
│ ├── poetry.py
│ ├── pyproject
│ │ ├── exceptions.py
│ │ ├── __init__.py
│ │ ├── tables.py
│ │ └── toml.py
│ ├── py.typed
│ ├── semver
│ │ ├── empty_constraint.py
│ │ ├── exceptions.py
│ │ ├── helpers.py
│ │ ├── __init__.py
│ │ ├── patterns.py
│ │ ├── version_constraint.py
│ │ ├── version.py
│ │ ├── version_range_constraint.py
│ │ ├── version_range.py
│ │ └── version_union.py
│ ├── spdx
│ │ ├── data
│ │ │ └── licenses.json
│ │ ├── helpers.py
│ │ ├── __init__.py
│ │ ├── license.py
│ │ └── updater.py
│ ├── toml
│ │ ├── exceptions.py
│ │ ├── file.py
│ │ └── __init__.py
│ ├── utils
│ │ ├── _compat.py
│ │ ├── helpers.py
│ │ ├── __init__.py
│ │ ├── patterns.py
│ │ └── toml_file.py
│ ├── vcs
│ │ ├── git.py
│ │ └── __init__.py
│ ├── _vendor
│ └── version
│ ├── exceptions.py
│ ├── grammars
│ │ ├── __init__.py
│ │ ├── markers.lark
│ │ └── pep508.lark
│ ├── helpers.py
│ ├── __init__.py
│ ├── markers.py
│ ├── parser.py
│ ├── pep440
│ │ ├── __init__.py
│ │ ├── parser.py
│ │ ├── segments.py
│ │ └── version.py
│ └── requirements.py
└── tests
26 directories, 88 files
Title
wheel/
├── poetry
│ └── core
│ ├── exceptions
│ │ ├── base.py
│ │ └── __init__.py
│ ├── factory.py
│ ├── __init__.py
│ ├── json
│ │ ├── __init__.py
│ │ └── schemas
│ │ └── poetry-schema.json
│ ├── lock
│ ├── masonry
│ │ ├── api.py
│ │ ├── builder.py
│ │ ├── builders
│ │ │ ├── builder.py
│ │ │ ├── __init__.py
│ │ │ ├── sdist.py
│ │ │ └── wheel.py
│ │ ├── __init__.py
│ │ ├── metadata.py
│ │ └── utils
│ │ ├── helpers.py
│ │ ├── include.py
│ │ ├── __init__.py
│ │ ├── module.py
│ │ └── package_include.py
│ ├── packages
│ │ ├── constraints
│ │ │ ├── any_constraint.py
│ │ │ ├── base_constraint.py
│ │ │ ├── constraint.py
│ │ │ ├── empty_constraint.py
│ │ │ ├── __init__.py
│ │ │ ├── multi_constraint.py
│ │ │ └── union_constraint.py
│ │ ├── dependency_group.py
│ │ ├── dependency.py
│ │ ├── directory_dependency.py
│ │ ├── file_dependency.py
│ │ ├── __init__.py
│ │ ├── package.py
│ │ ├── project_package.py
│ │ ├── specification.py
│ │ ├── url_dependency.py
│ │ ├── utils
│ │ │ ├── __init__.py
│ │ │ ├── link.py
│ │ │ └── utils.py
│ │ └── vcs_dependency.py
│ ├── poetry.py
│ ├── pyproject
│ │ ├── exceptions.py
│ │ ├── __init__.py
│ │ ├── tables.py
│ │ └── toml.py
│ ├── py.typed
│ ├── semver
│ │ ├── empty_constraint.py
│ │ ├── exceptions.py
│ │ ├── helpers.py
│ │ ├── __init__.py
│ │ ├── patterns.py
│ │ ├── version_constraint.py
│ │ ├── version.py
│ │ ├── version_range_constraint.py
│ │ ├── version_range.py
│ │ └── version_union.py
│ ├── spdx
│ │ ├── data
│ │ │ └── licenses.json
│ │ ├── helpers.py
│ │ ├── __init__.py
│ │ ├── license.py
│ │ └── updater.py
│ ├── toml
│ │ ├── exceptions.py
│ │ ├── file.py
│ │ └── __init__.py
│ ├── utils
│ │ ├── _compat.py
│ │ ├── helpers.py
│ │ ├── __init__.py
│ │ ├── patterns.py
│ │ └── toml_file.py
│ ├── vcs
│ │ ├── git.py
│ │ └── __init__.py
│ ├── _vendor
│ └── version
│ ├── exceptions.py
│ ├── grammars
│ │ ├── __init__.py
│ │ ├── markers.lark
│ │ └── pep508.lark
│ ├── helpers.py
│ ├── __init__.py
│ ├── markers.py
│ ├── parser.py
│ ├── pep440
│ │ ├── __init__.py
│ │ ├── parser.py
│ │ ├── segments.py
│ │ └── version.py
│ └── requirements.py
└── poetry_core-1.1.0b1.dist-info
├── LICENSE
├── METADATA
├── RECORD
└── WHEEL
24 directories, 87 files
Ahhhh, i see, that makes sense.
with a source tarball, you want the code layout to match the actual project source code layout so that setup.py can build it correctly. Looks like my latest commits don't fully solve the problem... :(
# Generated setup.py
...
package_dir = \
{'': '../../src'}
...
Also, I think I found an unrelated bug. When building sdists for pep420 namespace packages, the packages list in the generated setup.py file doesn't include the namespace package. Ex:
# pyproject.toml
#...
packages = [
{ include = "utsc/core", from = "../../src" }
]
#...
# Generated setup.py
...
packages = \
['core',
'core._vendor',
# etc, etc
'core.toml',
'core.yaml']
Kudos, SonarCloud Quality Gate passed! 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell
No Coverage information
0.0% Duplication
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @alextremblay . I would love to pick this up as I am in need of including "non-sub path" packages. Can I ask what needs to be done to get the failing test to pass? Thanks in advance.
Hi @alextremblay . I would love to pick this up as I am in need of including "non-sub path" packages. Can I ask what needs to be done to get the failing test to pass? Thanks in advance.
Speaking a little bit for @alextremblay, this is more blocked on design issues than missing/failing tests right now. We need to figure out how we can generate source distributions when we are including non-child directories as dependencies. Those sources do need to be bundled for a working sdist sand our current archive/sdist model assumes that all path dependencies are child directories.
Speaking a little bit for @alextremblay, this is more blocked on design issues than missing/failing tests right now. We need to figure out how we can generate source distributions when we are including non-child directories as dependencies. Those sources do need to be bundled for a working sdists and our current archive/sdist model assumes that all path dependencies are child directories.
the solution to that problem is to copy the contents of those external source trees into the root of the sdist and instruct poetry-core to use the package source tree at the root of the sdist when building from an sdist. As a matter of fact, my PR, as it stands now, already does this. it already "does the right thing" when building an sdist with poetry and installing that sdist with a PEP517 frontend (pip version >= 19.0).
The trouble is, poetry also auto-generates a setup.py script for non-pep517 build frontends, and that part isn't working. That's the test that's currently failing. i have not been able to find a way to fix that issue without breaking some other edge case in the poetry test suite.
If anybody has any ideas on how to achieve this, i'd be interested to find out.
In the mean time, i've switched to using hatch/hatchling as my build frontend / backend:
[build-system]
requires = ["hatchling"]
build-backend = "hatchling.build"
# requires = ["poetry-core @ git+https://github.com/atremblay/poetry-core.git@fix-issue-5621"]
# build-backend = "poetry.core.masonry.api"
[tool.hatch.build]
packages = ["utsc_nautobot"]
dev-mode-dirs = ["../../src"]
I miss poetry and the workflow I'm used to, but i can't make poetry work for my particular usecase
Thanks @alextremblay and @neersighted . I apologize if this might be off-topic but it might also benefit someone finding their way to this issue: I was able to include a source dir that is outside the project dir by switching over to hatch and using its "force-include" configuration option.
also want to chime in here, as I'm hitting the same limitation
in my use case, I've developed a go library that we also want to use as python extension wheels work fine, and that's nice, but sdist is not easy to achieve, as the go code is outside of the python subpath we have in our project, and the go code is needed for building the package from source
I know it's probably not the most common use case out there, but it would be awesome if I could still use poetry for this use case
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
0.0% Duplication
Closing as this is stalled and https://github.com/python-poetry/poetry-core/pull/273 aims to solve the same issues from a slightly different direction. There are still lots of semantics and packaging/interoperability-related questions to answer, but the author seems quite interested in solving them.