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

pkgs.formats.toml: fix TOML semantics by upgrading tomlkit #245440

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

luizribeiro
Copy link
Contributor

@luizribeiro luizribeiro commented Jul 25, 2023

Description of changes

This fixes #237521 by upgrading tomlkit to 0.11.8.

It also introduces tests to make sure this behavior is kept, as some of our configurations (such as telegraf) depend on it.

Tested these changes by running nix build .\#tests.pkgs-lib.formats.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Also, does this need to be backported?

Comment on lines 13 to 16
version = "0.11.6";
version = "0.11.4";
Copy link
Member

Choose a reason for hiding this comment

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

Downgrading packages should be exceptional: Is the new behavior a bug or a feature?

  • If it's a feature we should either
    • Introduce a new tomlkit_11_4 attribute and use that, or
    • Define the older version internally to the pkgs.formats code (this way we don't have a new attribute to maintain)
  • If it's a bug we should add a comment here saying that we're waiting for the bug to get fixed before upgrading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right :)

It turns out that tomlkit 0.11.8 (the latest version) doesn't have this bug either. So doing an upgrade and introducing the tests seems way more appropriate.

The challenge is that tomlkit is now built with poetry and.... poetry also depends on tomlkit.

So I think until we bootstrap poetry itself, we need to keep 0.11.6 as tomlkit_11_6, similar to your suggestion.

I'll make those changes. Hopefully I don't mess it up :-)

Thanks for the quick review, btw!

@luizribeiro
Copy link
Contributor Author

Not sure if this is the cleanest way to go about this, so I'm open to suggestions.

Also, does this need to be backported?

We need to have it merged into the 23.05 release, if that's what you are asking (sorry, I'm not yet very familiar with the nixpkgs release process / terminology).

@luizribeiro luizribeiro changed the title pkgs.formats.toml: fix TOML semantics by pinning tomlkit pkgs.formats.toml: fix TOML semantics by upgrading tomlkit Jul 25, 2023
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 25, 2023
@ofborg ofborg bot requested a review from jonringer July 25, 2023 22:18
@luizribeiro luizribeiro changed the title pkgs.formats.toml: fix TOML semantics by upgrading tomlkit pkgs.formats.toml: fix TOML semantics by pinning tomlkit Jul 26, 2023
@luizribeiro
Copy link
Contributor Author

Alright I'm calling it a day on this for now. Thanks for the help and sorry for all the churn :)

I've added a commit to my branch which has option 3 implemented that I mentioned above (a patch which makes tomlkit be built with setuptools instead of poetry).

I'll squash / drop those commits depending on the solution we go with before we merge this in.

@luizribeiro luizribeiro changed the title pkgs.formats.toml: fix TOML semantics by pinning tomlkit pkgs.formats.toml: fix TOML semantics by upgrading tomlkit Jul 27, 2023
@luizribeiro
Copy link
Contributor Author

After discussing this upstream on python-poetry/tomlkit#305, it turns out that poetry-core should not depend on tomlkit. poetry-core has no dependencies on other libraries and should be used to build tomlkit instead of poetry.

I've updated this PR accordingly.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Nice! Could you split this into separate commits like this?

  1. Remove poetry dependency on tomlkit and set `format = "pyproject" on tomlkit
  2. Update tomlkit to 0.11.8
  3. Introduce the test case

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

@infinisil infinisil merged commit b5f48a3 into NixOS:master Jul 27, 2023
4 checks passed
@mweinelt
Copy link
Member

f689956 broke pylint and by extension all nixos tests.

@vcunat
Copy link
Member

vcunat commented Jul 27, 2023

Note that this PR was tagged by OfBorg as very large rebuild and thus it should not have been merged to master.

@infinisil
Copy link
Member

Ah sorry I forgot to pay attention the label. Here's a Hydra failure for reference, also fails on x86_64-linux. After some quick error searching it looks like this commit in tomlkit introduced that error, and it's only included in 0.11.8, not 0.11.7. I can confirm that only upgrading to 0.11.7 fixes the pylint build.

@infinisil
Copy link
Member

Looks like pylint is aware of this problem: pylint-dev/pylint#8633

@mweinelt
Copy link
Member

Picked into #244135.

@infinisil infinisil mentioned this pull request Aug 30, 2023
12 tasks
rycee added a commit to nix-community/home-manager that referenced this pull request Aug 30, 2023
The generated format seems to have changed with the merge of
<NixOS/nixpkgs#245440>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pkgs.formats.toml changed semantics in 23.05
4 participants