libexpr: Canonicalize TOML timestamps for toml11 > 4.0#13741
libexpr: Canonicalize TOML timestamps for toml11 > 4.0#13741Mic92 merged 6 commits intoNixOS:masterfrom
Conversation
Current test suite doesn't cover the subsecond formatting at all and toml11 is quite finicky with that. We should at the very least test its behavior to avoid silent breakages on updates.
This looks really weird after the reformat.
There's no reason to use a std::function for recursive lambdas since there are polymorphic lambdas.
This addresses several changes from toml11 4.0 bump in nixpkgs [1]. 1. Added more regression tests for timestamp formats. Special attention needs to be paid to the precision of the subsecond range for local-time. Prior versions select the closest (upwards) multiple of 3 with a hard cap of 9 digits. 2. Normalize local datetime and offset datetime to always use the uppercase separator `T`. This is actually the issue surfaced in [2]. This canonicalization is basically a requirement by (a certain reading) of rfc3339 section 5.6 [3]. 3. If using toml11 >= 4.0 also keep the old behavior wrt to the number of digits used for subsecond part of the local-time. Newer versions cap it at 6 digits unconditionally. [1]: https://www.github.com/NixOS/nixpkgs/pull/331649 [2]: https://www.github.com/NixOS/nix/issues/11441 [3]: https://datatracker.ietf.org/doc/html/rfc3339
|
My opinion is that this feature is just broken: This is either throwing away information from the TOML document, or, very generously, introducing a Nix‐specific syntactic extension to TOML that allows specifying timestamps as tables in a way that disables all validation for them. No code that consumes a timestamp can assume anything about its value without rejecting valid TOML documents: Given this issue, the feature’s experimental status, the TOML spec’s refusal to pin down exact semantics for timestamps, and the demonstrated willingness of TOML implementations to change their behaviour, I feel it should be removed until someone can come up with semantics for it that make sense (e.g. annotating every node with a type, rather than just timestamps). I can understand wanting to decouple that decision from unblocking the It’s hard to search for users of a feature like this but the GitHub results for the feature name mostly seem like people going down the list of experimental features and enabling everything that seems like it might be a good idea. I’d guess nobody is relying on it. Maybe time to cut the losses on this one and free up the feature for someone to re‐do properly in the future. All that said, it seems good to apply a change like this to 2.24 regardless. |
|
cc @andreabedini – did you end up using this? I see that input-output-hk/foliage#46 is still open. A plugin would be another option in lieu of stable semantics here. |
|
We can discuss this today. |
|
From the nix team meeting:
|
This would have allowed us to catch to fromTOML regression in [1] without waiting for the dogfooding on master, since previously these tests [2] were not run for the Nix/Lix under test - only the host nix. [1]: NixOS/nix#13741 [2]: NixOS#433710
Current test suite doesn't cover the subsecond formatting at all and toml11 is quite finicky with that. We should at the very least test its behavior to avoid silent breakages on updates. (cherry picked from commit 7ed0229d1abd4414144c7af396842462ce6fc1eb) Upstream-PR: NixOS/nix#13741 Change-Id: I6a6a696433b168072d6ad2585dce8a3c10ccbc39
This looks really weird after the reformat. (cherry picked from commit df4e55ffc13c413e270af134227115a20a2341ba) Upstream-PR: NixOS/nix#13741 Change-Id: I8de92d58620cc4545a31d8b7d533d2f1e9f4f233
There's no reason to use a std::function for recursive lambdas since there are polymorphic lambdas. (cherry picked from commit a80a5c4dba0d944fab8f5ed57a343869ae96bf16) Upstream-PR: NixOS/nix#13741 Change-Id: I593bd04597e2ae000374ca1eca4d8928e986c0b5
(cherry picked from commit d8fc55a46e0c09241131097dbf1d6fa09e0a9808) Upstream-PR: NixOS/nix#13741 Change-Id: I8a11e21ae3bff3a885e13fbab74e1deb162a34cf
This addresses several changes from toml11 4.0 bump in nixpkgs [1]. 1. Added more regression tests for timestamp formats. Special attention needs to be paid to the precision of the subsecond range for local-time. Prior versions select the closest (upwards) multiple of 3 with a hard cap of 9 digits. 2. Normalize local datetime and offset datetime to always use the uppercase separator `T`. This is actually the issue surfaced in [2]. This canonicalization is basically a requirement by (a certain reading) of rfc3339 section 5.6 [3]. 3. If using toml11 >= 4.0 also keep the old behavior wrt to the number of digits used for subsecond part of the local-time. [1]: https://www.github.com/NixOS/nixpkgs/pull/331649 [2]: https://www.github.com/NixOS/nix/issues/11441 [3]: https://datatracker.ietf.org/doc/html/rfc3339 (cherry picked from commit dc769d72cb8ad22a0f89768682b5499a9d2b3d8b) Upstream-PR: NixOS/nix#13741 Change-Id: Iac4fbe5108be79be585e9670fa42dfd11f3c5e89
See [my comment] on the Nix PR to restore the previous behaviour for why I believe we should remove this for the next release. The PR should still be backported to stable releases to avoid making breaking changes to their semantics. [my comment]: <NixOS/nix#13741 (comment)> Fixing this across supported Lix versions is required for Nixpkgs to update toml11, which is a blocker for the CMake 4 update. Change-Id: I6a6a69642e6b6cb13a9fccc0778e9158b53102d5
This would have allowed us to catch to fromTOML regression in [1] without waiting for the dogfooding on master, since previously these tests [2] were not run for the Nix/Lix under test - only the host nix. [1]: NixOS/nix#13741 [2]: #433710 (cherry picked from commit 67ef265)
This would have allowed us to catch to fromTOML regression in [1] without waiting for the dogfooding on master, since previously these tests [2] were not run for the Nix/Lix under test - only the host nix. [1]: NixOS/nix#13741 [2]: NixOS/nixpkgs#433710 (cherry picked from commit 67ef2657ff6e3e8ece62a7a08b4231573a052a15)
…3741 libexpr: Canonicalize TOML timestamps for toml11 > 4.0 (backport #13741)
…3741 libexpr: Canonicalize TOML timestamps for toml11 > 4.0 (backport #13741)
…3741 libexpr: Canonicalize TOML timestamps for toml11 > 4.0 (backport #13741)
…3741 libexpr: Canonicalize TOML timestamps for toml11 > 4.0 (backport #13741)
This reverts commit 75740fb.
Reapply "Merge pull request #13741 from xokdvium/toml-timestamps"
Current test suite doesn't cover the subsecond formatting at all and toml11 is quite finicky with that. We should at the very least test its behavior to avoid silent breakages on updates. (cherry picked from commit 7ed0229d1abd4414144c7af396842462ce6fc1eb) Upstream-PR: NixOS/nix#13741 Change-Id: I6a6a696433b168072d6ad2585dce8a3c10ccbc39 (cherry picked from commit b2e48aa)
This looks really weird after the reformat. (cherry picked from commit df4e55ffc13c413e270af134227115a20a2341ba) Upstream-PR: NixOS/nix#13741 Change-Id: I8de92d58620cc4545a31d8b7d533d2f1e9f4f233 (cherry picked from commit 2ca5670)
There's no reason to use a std::function for recursive lambdas since there are polymorphic lambdas. (cherry picked from commit a80a5c4dba0d944fab8f5ed57a343869ae96bf16) Upstream-PR: NixOS/nix#13741 Change-Id: I593bd04597e2ae000374ca1eca4d8928e986c0b5 (cherry picked from commit 5badc1b)
(cherry picked from commit d8fc55a46e0c09241131097dbf1d6fa09e0a9808) Upstream-PR: NixOS/nix#13741 Change-Id: I8a11e21ae3bff3a885e13fbab74e1deb162a34cf (cherry picked from commit 19d9a87)
This addresses several changes from toml11 4.0 bump in nixpkgs [1]. 1. Added more regression tests for timestamp formats. Special attention needs to be paid to the precision of the subsecond range for local-time. Prior versions select the closest (upwards) multiple of 3 with a hard cap of 9 digits. 2. Normalize local datetime and offset datetime to always use the uppercase separator `T`. This is actually the issue surfaced in [2]. This canonicalization is basically a requirement by (a certain reading) of rfc3339 section 5.6 [3]. 3. If using toml11 >= 4.0 also keep the old behavior wrt to the number of digits used for subsecond part of the local-time. [1]: https://www.github.com/NixOS/nixpkgs/pull/331649 [2]: https://www.github.com/NixOS/nix/issues/11441 [3]: https://datatracker.ietf.org/doc/html/rfc3339 (cherry picked from commit dc769d72cb8ad22a0f89768682b5499a9d2b3d8b) Upstream-PR: NixOS/nix#13741 Change-Id: Iac4fbe5108be79be585e9670fa42dfd11f3c5e89 (cherry picked from commit 2898b9e)
Sorry for the long radio silence. How would a plugin work? If there a setup I can copy? |
This would have allowed us to catch to fromTOML regression in [1] without waiting for the dogfooding on master, since previously these tests [2] were not run for the Nix/Lix under test - only the host nix. [1]: NixOS/nix#13741 [2]: NixOS/nixpkgs#433710
Current test suite doesn't cover the subsecond formatting at all and toml11 is quite finicky with that. We should at the very least test its behavior to avoid silent breakages on updates. (cherry picked from commit 7ed0229d1abd4414144c7af396842462ce6fc1eb) Upstream-PR: NixOS/nix#13741 Change-Id: I6a6a696433b168072d6ad2585dce8a3c10ccbc39
Motivation
This addresses several changes from toml11 4.0 bump in
nixpkgs 1.
Added more regression tests for timestamp formats.
Special attention needs to be paid to the precision
of the subsecond range for local-time. Prior versions select the closest
(upwards) multiple of 3 with a hard cap of 9 digits.
Normalize local datetime and offset datetime to always
use the uppercase separator
T. This is actually the issuesurfaced in 2. This canonicalization is basically a requirement
by (a certain reading) of rfc3339 section 5.6 3.
If using toml11 >= 4.0 also keep the old behavior wrt
to the number of digits used for subsecond part of the local-time.
Newer versions
cap it at 6 digits unconditionallypreserve the input.Context
Fixes #11441
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.