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

Remove tomlkit regression mitigation #5675

Closed
abn opened this issue May 23, 2022 · 5 comments · Fixed by #5870
Closed

Remove tomlkit regression mitigation #5675

abn opened this issue May 23, 2022 · 5 comments · Fixed by #5870
Labels
area/windows For Windows-specific issues
Milestone

Comments

@abn
Copy link
Member

abn commented May 23, 2022

In #5673 we introduced a mitigation for a tomlkit regression on Windows. The root cause of this has to be identified, addressed and mitigation addressed.

Mitigation commit: 1d71955

@abn abn added this to the 1.2 milestone May 23, 2022
@abn abn mentioned this issue May 23, 2022
@Secrus Secrus added area/windows For Windows-specific issues Dependency labels May 23, 2022
@radoering
Copy link
Member

radoering commented Jun 14, 2022

Bisected the regression. It's because of python-poetry/tomlkit#149. I believe this change falls way too short. Tomlkit now keeps Windows line endings when reading but probably doesn't expect Windows line endings in some parts of the code. Further, if you read a file with Windows line endings, change it and write it back you may get mixed line endings (because "\n" used in code are not converted to "\r\n" anymore when writing). Even though the change fixes #2993, it makes things a lot worse for toml files with Windows line endings. Not sure, if there is an easy fix. Maybe, we should think about reverting this change in our vendored tomlkit in poetry-core.

@abn
Copy link
Member Author

abn commented Jun 14, 2022

Maybe, we should think about reverting this change in our vendored tomlkit in poetry-core.

Do not tihnk that is a viable option for a few reasons.

  1. The vendored version (in theory) will only impact core's use of tomlkit. Unfortunately, due to an import order inconsistencies Poetry may or may not load the right tomlkit (non-vendored) package.
  2. Downstream package maintainers often strip out vendored packages and provide them externally. Since core's vendoring use case is specific to ease of PEP 517 builds, we do not enforce the expectation of vendored package being used.
  3. This is a problematic pattern (carrying funcitonal patches), we want to avoid.

@abn
Copy link
Member Author

abn commented Jun 14, 2022

Further, if you read a file with Windows line endings, change it and write it back you may get mixed line endings (because "\n" used in code are not converted to "\r\n" anymore when writing).

Would replacing our \n usages with nl() or similar help here? Assuming tomlkit detects and is "aware" of the current file's line-ending preferences?

@radoering
Copy link
Member

Unfortunately, tomlkit is not "aware" of any preferences. Since python-poetry/tomlkit#149, it just reads the file with line endings as is and inserts new elements with "\n" (even if the file only contains "\r\n"). If I'm correct, a holistic fix in tomlkit to keep TOMLDocument consistent would require a lot of effort (since there are many hard coded "\n") and many new tests.

I created a PR so at least TOMLFile keeps line endings consistent. In other words, if you dump strings, the line endings in the strings will still be inconsistent, but if you edit a file, line endings stay consistent (if they were consistent before). For testing, I also created a PR in poetry-core that patches TOMLFile without need for a new tomlkit version. (That was quite simple because we already had our own subclass of the original TOMLFile.)

We can either wait for a tomlkit version with python-poetry/tomlkit#201 (or another fix for python-poetry/tomlkit#200) or proceed with python-poetry/poetry-core#403.

Copy link

github-actions bot commented Mar 1, 2024

This issue 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
area/windows For Windows-specific issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants