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.

Sign up for GitHub

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

Jump to bottom

Fix sdist build for multiple readme files #486

Merged
merged 9 commits into from
Jan 9, 2023
Merged

Fix sdist build for multiple readme files #486

merged 9 commits into from
Jan 9, 2023

Conversation

wagnerluis1982
Copy link
Contributor

@wagnerluis1982 wagnerluis1982 commented Oct 1, 2022 •
edited
Loading

Resolves: python-poetry/poetry#6633

I fixed the issue by changing Poetry.local_config["readme"] type to a sequence. In the sdist builder, I am adding all the files found in that sequence.

@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from c7b3441 to 0e7b2d2 Compare October 1, 2022 09:06
@wagnerluis1982
Copy link
Contributor Author

Downstream tests failed because of the change in local_config.

Before I implemented the current solution, I wondered if I should handle the multi-type (str | Sequence) nature of readme in sdist, which would solve the downstream test problem, but I wonder if this would not make recurring to inspect readme type in other builders, if appears.

neersighted
neersighted requested changes Oct 1, 2022
View reviewed changes
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this fix is acceptable as-is -- in factory.py we explicitly handle the case of the readmes being a string or some other sequence (tuple/list), and I don't think it's unreasonable to do such downstream given it's in the schema.

The special casing and in-place mutation here is pretty gross and surprising, and hard to maintain. Let's properly handle both cases in the sdist builder, and then add some integration tests in Poetry.

@wagnerluis1982
Copy link
Contributor Author

I don't think this fix is acceptable as-is -- in factory.py we explicitly handle the case of the readmes being a string or some other sequence (tuple/list), and I don't think it's unreasonable to do such downstream given it's in the schema.

I was expecting this 😆, thank you for the feedback. I will proceed that way.

@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch 2 times, most recently from 9210d73 to 0d57c91 Compare October 1, 2022 16:58
neersighted
neersighted requested changes Oct 1, 2022
View reviewed changes
tests/masonry/builders/test_sdist.py Outdated Show resolved Hide resolved
neersighted
neersighted previously requested changes Oct 1, 2022
View reviewed changes
tests/masonry/builders/test_sdist.py Outdated Show resolved Hide resolved
src/poetry/core/masonry/builders/sdist.py Outdated Show resolved Hide resolved
src/poetry/core/masonry/builders/sdist.py Outdated Show resolved Hide resolved
@wagnerluis1982 wagnerluis1982 mentioned this pull request Oct 2, 2022
Merged
@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from de95c6d to 7ebd087 Compare October 3, 2022 22:32
@wagnerluis1982
Copy link
Contributor Author

@neersighted I added downstream tests to python-poetry/poetry#6678.

If you have something else in mind regarding this PR, let me know :-)

@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch 2 times, most recently from c6df4ef to 626f5e7 Compare October 12, 2022 10:07
@wagnerluis1982
Copy link
Contributor Author

Any chance this is merged this week? 😊

@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from 626f5e7 to 78e1bc4 Compare October 16, 2022 17:38
@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from 78e1bc4 to b7048ad Compare October 23, 2022 20:03
@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch 2 times, most recently from 2b71abc to b8324b1 Compare November 1, 2022 18:27
@wagnerluis1982
Copy link
Contributor Author

@neersighted this PR has exact 1 month. I've been told the next poetry release is nigh. How about to continue the discussions for this fix be included?

@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from b8324b1 to baeb8b0 Compare November 5, 2022 09:07
@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from baeb8b0 to b07b707 Compare November 19, 2022 04:26
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch 2 times, most recently from a6c7705 to 8e0f333 Compare December 21, 2022 18:57
@wagnerluis1982
Copy link
Contributor Author

@neersighted would you be able to check this PR? 🎅

@wagnerluis1982
refactor tests
f793478
@wagnerluis1982
refactoring
358a4c3
@wagnerluis1982
drop meaningless parenthesis
c372284
@wagnerluis1982
simplify typing
faa49bd 
- connsider `local_config["readme"]` as `str | Iterable[str]` in sdist
@wagnerluis1982 wagnerluis1982 force-pushed the bugfix/sdist-build-with-multiple-readme-files branch from 8e0f333 to faa49bd Compare December 30, 2022 17:01
@wagnerluis1982 wagnerluis1982 mentioned this pull request Dec 30, 2022
Closed
4 tasks
radoering
radoering requested changes Jan 7, 2023
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, there seem to be only minor concerns here. I think with some small changes this can be merged.

src/poetry/core/masonry/builders/sdist.py Outdated Show resolved Hide resolved
src/poetry/core/masonry/builders/sdist.py Outdated Show resolved Hide resolved
src/poetry/core/masonry/builders/sdist.py Outdated Show resolved Hide resolved
@wagnerluis1982
Copy link
Contributor Author

@radoering

All in all, there seem to be only minor concerns here. I think with some small changes this can be merged.

Thank you for the suggestions, I just applied them 🙂

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@radoering radoering dismissed neersighted’s stale review January 9, 2023 04:49

additional_files is set[Path] now (instead of set[Path | str])

@radoering radoering merged commit cd5b624 into python-poetry:main Jan 9, 2023
@wagnerluis1982 wagnerluis1982 deleted the bugfix/sdist-build-with-multiple-readme-files branch January 10, 2023 05:14
@radoering radoering mentioned this pull request Jan 24, 2023
Merged
@radoering radoering mentioned this pull request Feb 18, 2023
Merged
mwalbeck pushed a commit to mwalbeck/docker-python-poetry that referenced this pull request Feb 28, 2023
@mwalbeck
Update dependency poetry to v1.4.0 (#655)
a676e18 
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [poetry](https://python-poetry.org/) ([source](https://github.com/python-poetry/poetry), [changelog](https://python-poetry.org/history/)) | minor | `1.3.2` -> `1.4.0` |

---

### Release Notes

<details>
<summary>python-poetry/poetry</summary>

### [`v1.4.0`](https://github.com/python-poetry/poetry/blob/HEAD/CHANGELOG.md#&#8203;140---2023-02-27)

[Compare Source](python-poetry/poetry@1.3.2...1.4.0)

##### Added

-   **Add a modern installer (`installer.modern-installation`) for faster installation of packages and independence from pip** ([#&#8203;6205](python-poetry/poetry#6205)).
-   Add support for `Private ::` trove classifiers ([#&#8203;7271](python-poetry/poetry#7271)).
-   Add the version of poetry in the `@generated` comment at the beginning of the lock file ([#&#8203;7339](python-poetry/poetry#7339)).
-   Add support for `virtualenvs.prefer-active-python` when running `poetry new` and `poetry init` ([#&#8203;7100](python-poetry/poetry#7100)).

##### Changed

-   **Deprecate the old installer, i.e. setting `experimental.new-installer` to `false`** ([#&#8203;7358](python-poetry/poetry#7358)).
-   Remove unused `platform` field from cached package info and bump the cache version ([#&#8203;7304](python-poetry/poetry#7304)).
-   Extra dependencies of the root project are now sorted in the lock file ([#&#8203;7375](python-poetry/poetry#7375)).
-   Remove upper boundary for `importlib-metadata` dependency ([#&#8203;7434](python-poetry/poetry#7434)).
-   Validate path dependencies during use instead of during construction ([#&#8203;6844](python-poetry/poetry#6844)).
-   Remove the deprecated `repository` modules ([#&#8203;7468](python-poetry/poetry#7468)).

##### Fixed

-   Fix an issue where an unconditional dependency of an extra was not installed in specific environments ([#&#8203;7175](python-poetry/poetry#7175)).
-   Fix an issue where a pre-release of a dependency was chosen even if a stable release fulfilled the constraint ([#&#8203;7225](python-poetry/poetry#7225), [#&#8203;7236](python-poetry/poetry#7236)).
-   Fix an issue where HTTP redirects were not handled correctly during publishing ([#&#8203;7160](python-poetry/poetry#7160)).
-   Fix an issue where `poetry check` did not handle the `-C, --directory` option correctly ([#&#8203;7241](python-poetry/poetry#7241)).
-   Fix an issue where the subdirectory information of a git dependency was not written to the lock file ([#&#8203;7367](python-poetry/poetry#7367)).
-   Fix an issue where the wrong Python version was selected when creating an virtual environment ([#&#8203;7221](python-poetry/poetry#7221)).
-   Fix an issue where packages that should be kept were uninstalled when calling `poetry install --sync` ([#&#8203;7389](python-poetry/poetry#7389)).
-   Fix an issue where an incorrect value was set for `sys.argv[0]` when running installed scripts ([#&#8203;6737](python-poetry/poetry#6737)).
-   Fix an issue where hashes in `direct_url.json` files were not written according to the specification ([#&#8203;7475](python-poetry/poetry#7475)).
-   Fix an issue where poetry commands failed due to special characters in the path of the project or virtual environment ([#&#8203;7471](python-poetry/poetry#7471)).
-   Fix an issue where poetry crashed with a `JSONDecodeError` when running a Python script that produced certain warnings ([#&#8203;6665](python-poetry/poetry#6665)).

##### Docs

-   Add advice on how to maintain a poetry plugin ([#&#8203;6977](python-poetry/poetry#6977)).
-   Update tox examples to comply with the latest tox release ([#&#8203;7341](python-poetry/poetry#7341)).
-   Mention that the `poetry export` can export `constraints.txt` files ([#&#8203;7383](python-poetry/poetry#7383)).
-   Add clarifications for moving configuration files ([#&#8203;6864](python-poetry/poetry#6864)).
-   Mention the different types of exact version specifications ([#&#8203;7503](python-poetry/poetry#7503)).

##### poetry-core ([`1.5.1`](https://github.com/python-poetry/poetry-core/releases/tag/1.5.1))

-   Improve marker handling ([#&#8203;528](python-poetry/poetry-core#528),
    [#&#8203;534](python-poetry/poetry-core#534),
    [#&#8203;530](python-poetry/poetry-core#530),
    [#&#8203;546](python-poetry/poetry-core#546),
    [#&#8203;547](python-poetry/poetry-core#547)).
-   Validate whether dependencies referenced in `extras` are defined in the main dependency group ([#&#8203;542](python-poetry/poetry-core#542)).
-   Poetry no longer generates a `setup.py` file in sdists by default ([#&#8203;318](python-poetry/poetry-core#318)).
-   Fix an issue where trailing newlines were allowed in `tool.poetry.description` ([#&#8203;505](python-poetry/poetry-core#505)).
-   Fix an issue where the name of the data folder in wheels was not normalized ([#&#8203;532](python-poetry/poetry-core#532)).
-   Fix an issue where the order of entries in the RECORD file was not deterministic ([#&#8203;545](python-poetry/poetry-core#545)).
-   Fix an issue where zero padding was not correctly handled in version comparisons ([#&#8203;540](python-poetry/poetry-core#540)).
-   Fix an issue where sdist builds did not support multiple READMEs ([#&#8203;486](python-poetry/poetry-core#486)).

##### poetry-plugin-export ([`^1.3.0`](https://github.com/python-poetry/poetry-plugin-export/releases/tag/1.3.0))

-   Fix an issue where the export failed if there was a circular dependency on the root package ([#&#8203;118](python-poetry/poetry-plugin-export#118)).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNTIuNSIsInVwZGF0ZWRJblZlciI6IjM0LjE1Mi41In0=-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-python-poetry/pulls/655
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radoering radoering radoering approved these changes

@neersighted neersighted Awaiting requested review from neersighted

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

poetry build fails sdist build when using multiple readme entries
3 participants
@wagnerluis1982 @neersighted @radoering