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

Fix generate_system_pyproject newline issues #7705

Merged

Conversation

brooke-ec
Copy link
Contributor

@brooke-ec brooke-ec commented Mar 22, 2023

Addressed Issues

Additional Carriage Returns

tomlkit persists the original line separators for the preserved tables, as a result these lines will be separated by \r\n on WIndows. Since the newline kwarg is omitted from the self.system_pyproject.write_text[1] method call, all occurrences of \n are replaced by os.linesep[2] before writing. This causes the resultant line separators for the preserved lines to be \r\r\n on Windows.

These additional carriage returns become an issue when the file is parsed by tomli, which replaces only \r\n with \n,[3] leaving the extra return. When encountering this, the following exception will be raised: TOMLDecodeError: Expected newline or end of document after a statement (at line 12, column 44).

This issue causes the following commands to prematurely exit on Windows when any plugins are installed: self lock, self show, self update and self show plugins.

Increasing Lines at End of File

tomlkit also persists the number of blank lines after an entry, as a result when preserved tables are present, the newline added by Factory.create_pyproject_from_package[4] is unnecessary and will cause a new line to be added to the file every time the function is called.

Changes To Fix The Issues

  1. Use TOMLFile.write for saving the file to let tomlkit handle the translation of newlines.
  2. Remove line that adds the trailing newline in Factory.create_pyproject_from_package.

Pull Request Check List

Resolves #6457

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

@dimbleby
Copy link
Contributor

Something like

-        self.system_pyproject.write_text(content.as_string(), encoding="utf-8")
+        pyproject = PyProjectTOML(self.system_pyproject)
+        pyproject.file.write(content)

should be cleaner, let tomlkit worry about the line endings. (Which it does, see https://github.com/sdispater/tomlkit/blob/18a527758bfa3f8fe65e2f45f61df42f2f5af0c1/tomlkit/toml_file.py#L47)

per the pipeline, write_text() doesn't get the newline parameter until python 3.10.

Removed newline kwarg from cmd.system_pyproject.write_text call in tests for compatibility with older versions.
@brooke-ec
Copy link
Contributor Author

Thanks for the help! That is definitely a cleaner solution. It's just a case of finding a nice solution to prevent those newlines from being added with this new method.

@dimbleby
Copy link
Contributor

well, one thing at a time. Although the extra lines and the faulty carriage returns are in the same general area they're really quite different; and one oughtn't cause us to make a worse fix to the other.

re the unwanted newlines: do we ever want that newline? perhaps just delete the line of code that adds it altogether. Or, make it caller responsibility to add it if they want it.

@brooke-ec
Copy link
Contributor Author

The newline appears to be added solely for code styling, so I'd say it is probably alright to remove. Aside from here, the Factory.create_pyproject_from_package method is only used in test_factory.py and test_remove_plugins.py, neither of which look like they will be effected by the removal of this newline.

NimajnebEC and others added 3 commits March 24, 2023 09:49
Removed newline kwarg from cmd.system_pyproject.write_text call in tests for compatibility with older versions.
@brooke-ec brooke-ec force-pushed the fix-generate-system-pyproject-newlines branch from 05ba928 to 523faba Compare March 24, 2023 09:49
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.

I'd propose to rename the test file to test_self_command.py. Usually, we use the same name as the module we are testing.

src/poetry/factory.py Outdated Show resolved Hide resolved
tests/console/commands/self/test_system_pyproject.py Outdated Show resolved Hide resolved
tests/console/commands/self/test_system_pyproject.py Outdated Show resolved Hide resolved
Co-authored-by: Randy Döring <30527984+radoering@users.noreply.github.com>
@dimbleby
Copy link
Contributor

fixes #6457, presumably

@radoering radoering merged commit 9f6f462 into python-poetry:master Mar 27, 2023
adriangb pushed a commit to adriangb/poetry that referenced this pull request Apr 5, 2023
mwalbeck pushed a commit to mwalbeck/docker-python-poetry that referenced this pull request May 23, 2023
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.4.2` -> `1.5.0` |

---

### Release Notes

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

### [`v1.5.0`](https://github.com/python-poetry/poetry/blob/HEAD/CHANGELOG.md#&#8203;150---2023-05-19)

[Compare Source](python-poetry/poetry@1.4.2...1.5.0)

##### Added

-   **Introduce the new source priorities `explicit` and `supplemental`** ([#&#8203;7658](python-poetry/poetry#7658),
    [#&#8203;6879](python-poetry/poetry#6879)).
-   **Introduce the option to configure the priority of the implicit PyPI source** ([#&#8203;7801](python-poetry/poetry#7801)).
-   Add handling for corrupt cache files ([#&#8203;7453](python-poetry/poetry#7453)).
-   Improve caching of URL and git dependencies ([#&#8203;7693](python-poetry/poetry#7693),
    [#&#8203;7473](python-poetry/poetry#7473)).
-   Add option to skip installing directory dependencies ([#&#8203;6845](python-poetry/poetry#6845),
    [#&#8203;7923](python-poetry/poetry#7923)).
-   Add `--executable` option to `poetry env info` ([#&#8203;7547](python-poetry/poetry#7547)).
-   Add `--top-level` option to `poetry show` ([#&#8203;7415](python-poetry/poetry#7415)).
-   Add `--lock` option to `poetry remove` ([#&#8203;7917](python-poetry/poetry#7917)).
-   Add experimental `POETRY_REQUESTS_TIMEOUT` option ([#&#8203;7081](python-poetry/poetry#7081)).
-   Improve performance of wheel inspection by avoiding unnecessary file copy operations ([#&#8203;7916](python-poetry/poetry#7916)).

##### Changed

-   **Remove the old deprecated installer and the corresponding setting `experimental.new-installer`** ([#&#8203;7356](python-poetry/poetry#7356)).
-   **Introduce `priority` key for sources and deprecate flags `default` and `secondary`** ([#&#8203;7658](python-poetry/poetry#7658)).
-   Deprecate `poetry run <entry point>` if the entry point was not previously installed via `poetry install` ([#&#8203;7606](python-poetry/poetry#7606)).
-   Only write the lock file if the installation succeeds ([#&#8203;7498](python-poetry/poetry#7498)).
-   Do not write the unused package category into the lock file ([#&#8203;7637](python-poetry/poetry#7637)).

##### Fixed

-   Fix an issue where Poetry's internal pyproject.toml continually grows larger with empty lines ([#&#8203;7705](python-poetry/poetry#7705)).
-   Fix an issue where Poetry crashes due to corrupt cache files ([#&#8203;7453](python-poetry/poetry#7453)).
-   Fix an issue where the `Retry-After` in HTTP responses was not respected and retries were handled inconsistently ([#&#8203;7072](python-poetry/poetry#7072)).
-   Fix an issue where Poetry silently ignored invalid groups ([#&#8203;7529](python-poetry/poetry#7529)).
-   Fix an issue where Poetry does not find a compatible Python version if not given explicitly ([#&#8203;7771](python-poetry/poetry#7771)).
-   Fix an issue where the `direct_url.json` of an editable install from a git dependency was invalid ([#&#8203;7473](python-poetry/poetry#7473)).
-   Fix an issue where error messages from build backends were not decoded correctly ([#&#8203;7781](python-poetry/poetry#7781)).
-   Fix an infinite loop when adding certain dependencies ([#&#8203;7405](python-poetry/poetry#7405)).
-   Fix an issue where pre-commit hooks skip pyproject.toml files in subdirectories ([#&#8203;7239](python-poetry/poetry#7239)).
-   Fix an issue where pre-commit hooks do not use the expected Python version ([#&#8203;6989](python-poetry/poetry#6989)).
-   Fix an issue where an unclear error message is printed if the project name is the same as one of its dependencies ([#&#8203;7757](python-poetry/poetry#7757)).
-   Fix an issue where `poetry install` returns a zero exit status even though the build script failed ([#&#8203;7812](python-poetry/poetry#7812)).
-   Fix an issue where an existing `.venv` was not used if `in-project` was not set ([#&#8203;7792](python-poetry/poetry#7792)).
-   Fix an issue where multiple extras passed to `poetry add` were not parsed correctly ([#&#8203;7836](python-poetry/poetry#7836)).
-   Fix an issue where `poetry shell` did not send a newline to `fish` ([#&#8203;7884](python-poetry/poetry#7884)).
-   Fix an issue where `poetry update --lock` printed operations that were not executed ([#&#8203;7915](python-poetry/poetry#7915)).
-   Fix an issue where `poetry add --lock` did perform a full update of all dependencies ([#&#8203;7920](python-poetry/poetry#7920)).
-   Fix an issue where `poetry shell` did not work with `nushell` ([#&#8203;7919](python-poetry/poetry#7919)).
-   Fix an issue where subprocess calls failed on Python 3.7 ([#&#8203;7932](python-poetry/poetry#7932)).
-   Fix an issue where keyring was called even though the password was stored in an environment variable ([#&#8203;7928](python-poetry/poetry#7928)).

##### Docs

-   Add information about what to use instead of `--dev` ([#&#8203;7647](python-poetry/poetry#7647)).
-   Promote semantic versioning less aggressively ([#&#8203;7517](python-poetry/poetry#7517)).
-   Explain Poetry's own versioning scheme in the FAQ ([#&#8203;7517](python-poetry/poetry#7517)).
-   Update documentation for configuration with environment variables ([#&#8203;6711](python-poetry/poetry#6711)).
-   Add details how to disable the virtualenv prompt ([#&#8203;7874](python-poetry/poetry#7874)).
-   Improve documentation on whether to commit `poetry.lock` ([#&#8203;7506](python-poetry/poetry#7506)).
-   Improve documentation of `virtualenv.create` ([#&#8203;7608](python-poetry/poetry#7608)).

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

-   Improve error message for invalid markers ([#&#8203;569](python-poetry/poetry-core#569)).
-   Increase robustness when deleting temporary directories on Windows ([#&#8203;460](python-poetry/poetry-core#460)).
-   Replace `tomlkit` with `tomli`, which changes the interface of some *internal* classes ([#&#8203;483](python-poetry/poetry-core#483)).
-   Deprecate `Package.category` ([#&#8203;561](python-poetry/poetry-core#561)).
-   Fix a performance regression in marker handling ([#&#8203;568](python-poetry/poetry-core#568)).
-   Fix an issue where wildcard version constraints were not handled correctly ([#&#8203;402](python-poetry/poetry-core#402)).
-   Fix an issue where `poetry build` created duplicate Python classifiers if they were specified manually ([#&#8203;578](python-poetry/poetry-core#578)).
-   Fix an issue where local versions where not handled correctly ([#&#8203;579](python-poetry/poetry-core#579)).

</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:eyJjcmVhdGVkSW5WZXIiOiIzNS44Mi4wIiwidXBkYXRlZEluVmVyIjoiMzUuODIuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-python-poetry/pulls/717
Co-authored-by: renovate-bot <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
@brooke-ec brooke-ec deleted the fix-generate-system-pyproject-newlines branch June 6, 2023 20:38
Copy link

github-actions bot commented Mar 3, 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 3, 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.

Poetry's internal pyproject.toml continually grows larger with empty lines
4 participants