Fix for including modules from different locations #108
Conversation
|
Thanks a lot for your contribution @jaharkes 👍 Could you please add a test for this fix? |
|
Thanks for looking at it, it looks like my test is hitting some case-sensitivity issue on windows. I guess I should have picked better names than 'moduleA' and 'moduleB'. |
tests/masonry/builders/test_sdist.py
Outdated
| setup_ast.body = [n for n in setup_ast.body if isinstance(n, ast.Assign)] | ||
| ns = {} | ||
| exec(compile(setup_ast, filename="setup.py", mode="exec"), ns) | ||
| assert "module_a" in ns["package_dir"] or "module_b" in ns["package_dir"] |
There was a problem hiding this comment.
(Thanks for putting this together - I was having the same issue with two packages lying in different folders.)
Why is there an or here? Shouldn't it always be that ns["package_dir"].keys() == {"", "module_b"}? If not, that'd mean that the build is not reproducible given the same input, which isn't good.
There was a problem hiding this comment.
Yes, I believe you are correct because the first entry in the list is module_a which will always be added as "".
I don't know if the result of dict().keys() is guaranteed to be ordered, but we can test for "" in ... and "module_b" in ...
There was a problem hiding this comment.
Well, you could just test for equality: ns["package_dir"] == {"": "lib_a", "module_b": "lib_b/module_b"}.
jaharkes
force-pushed
the
master
branch
2 times, most recently
from
9f7af18
to
7a26510
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
When we define multiple packages from different source locations, Poetry currently only uses the last specified from= location. This patch adds explicit paths to package_dir for additional packages. This fixes python-poetry/poetry#1811, fixes python-poetry/poetry#2354, and possibly even python-poetry/poetry#2450.
When we try to include moduleA from libA and moduleB from libB then package_dir in the generated setup.py must to contain either one or both `"moduleA": "libA/moduleA"` or `"moduleB": "libB/moduleB"` so we are able to find both modules when building the source dist.
As noted by @layday, `ns["package_dir"].keys() == {"", "module_b"}` should always be true, so we don't have to test for module_a in `ns["package_dir"]`.
|
@python-poetry/triage rebased against the current master and successfully ran the tox tests locally. |
There was a problem hiding this comment.
This change looks quite straightforward, and indeed addresses the bug in question with good test coverage. I'm going to let the checks run and give it a merge if they're good.
Thanks for your review @layday -- I think you addressed all the concerns I had with the original version.
When we define multiple packages from different source locations,
Poetry currently only uses the last specified from= location.
This patch adds explicit paths to package_dir for additional packages.
This fixes python-poetry/poetry#1811, fixes python-poetry/poetry#2354,
and possibly even python-poetry/poetry#2450.