Parse TOML timestamps as strings #8120

andreabedini · 2023-03-28T04:52:20Z

Motivation

Writing foliage I have choosen TOML knowing that nix would be able to read it natively. As it turns out, I cannot parse foliage input metadata because it includes timestamps, which nix does not support. 💔

Context

Currently fromTOML throws an exception when encountering a timestamp since the nix language lacks a way to represent them. This patch changes this beaviour and makes fromTOML parse timestamps as their were strings.

I believe this is the right thing to do here, given that a string is better than nothing.

Note that, ifwe had toTOML, ensuring fromTOML (toTOML x) == x might be impossible anyway because nix values can be expressed in multiple ways in TOML.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Ericson2314 · 2023-04-10T04:10:46Z

Discussed at last Nix team meeting.

Verdict: Let's do this, but in a way that round-trips properly, and as an experimental feature.

Expand details for an idea of how round-tripping could work.

- @edolstra: Probably the best we can do. The alternative would be encoding it in a structured way, which would be fairly complicated.

@roberth: Parsing it in a structured way might actually be cool. In particular for NixOS use-case.
Middle-ground: parse it as { _type = "timestamp"; value = "${theRawTSString}"; }
@Ericson2314: Should probably make it experimental, then we can refine it over time, and unblock the person without stabilizing anything rashly.
@roberth: Sure, but I don't mind stabilizing it sooner.

andreabedini · 2023-04-12T08:59:33Z

👍 I think we should follow the TOML's definitions (Offset Date-Time, Local Date-Time, Local Date, Local Date). These types are already available from toml++. AFAIK they are all subsets of RFC 3339 anyway.

With some luck I'll find some time to work on this week or the next.

nixos-discourse · 2023-04-17T09:44:35Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-04-10-nix-team-meeting-minutes-47/27357/1

edolstra · 2023-05-08T12:55:02Z

src/libexpr/primops/fromTOML.cc

-                throw std::runtime_error("Dates and times are not supported");
+            case toml::value_t::local_time: {
+                auto attrs = state.buildBindings(2);
+                attrs.alloc("_type").mkString("timestamp");


"timestamp" is a bit too generic, maybe it should be "toml-datetime" or something like that.

Also, maybe the specific TOML type (toml::value_t::{local_datetime,...}) should be stored in the attrset (either encoded in _type, or as a tomlType attribute). If we ever add a toTOML builtin, that should make it easier to round-trip.

Related: #3929

fricklerhandwerk · 2023-05-09T09:38:06Z

Reviewed in Nix team meeting 2023-05-08:

We still want this, but we also want to get it right. As @edolstra pointed out we have to take care of not occupying the _type namespace haphazardly, and also look out for round-tripping TOML correctly. We didn't come to a conclusion, and still owe a clear decision. Suggestions how to proceed are welcome.

Complete discussion

@edolstra: the attrset type should say rawTOML
- since _type is in a global namespace, no one else will be able to implement timestamps on top of that
- @roberth: agreed. calling it timestamp comes with expectations we may have to maintain code around
  - the only objection may be that it's a scary name
  - @edolstra: all we have to express that it will be returned verbatim when serialising TOML again
@edolstra: on the other hand, our TOML library can represent the different TOML time types, so maybe we can express this in the output
@roberth: not having a toTOML is a bit of a liability
- we wanted round-tripping, but currently have no way to test correctness
needs more discussion about how to proceed

fricklerhandwerk · 2023-05-09T09:39:19Z

My take on this is, since it's supposed to be guarded by an experimental feature, we could merge it in whatever form, and then adapt as needed once we have a toTOML implementation. @andreabedini in any case we'd need the experimental guard.

nixos-discourse · 2023-05-09T09:40:05Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-08-nix-team-meeting-minutes-53/27967/1

andreabedini · 2023-05-22T09:10:12Z

Thank you @fricklerhandwerk. I followed the rest of the code w.r.t. the experimental flag. Let me know if this is ok. I believe the CI failure is unrelated to the PR.

Currently fromTOML throws an exception when encountering a timestamp since the nix language lacks a way to represent them. This patch changes this beaviour and makes fromTOML parse timestamps as attrsets of the format { _type = "timestamp"; value = "1979-05-27T07:32:00Z"; }

…s-strings

fricklerhandwerk · 2023-05-31T08:49:10Z

src/libutil/experimental-features.cc

+        .tag = Xp::ParseTomlTimestamps,
+        .name = "parse-toml-timestamps",
+        .description = R"(
+            Allow parsing of timestamps in builtins.fromTOML.


I was just about to make a suggestion to link to builtins.fromTOML documentation, but that's not even documented at all.

That made me realise that we seem to have been talking about making all of builtins.fromTOML experimental in order to have some time to figure out the roundtrip design. @nixos/nix-team is that correct? @andreabedini sorry that whatever misunderstanding was unearthed here may produce even more work for you...

No problem ☺️

fricklerhandwerk · 2023-06-09T09:52:02Z

Discussed in the Nix team meeting 2023-06-05:

@fricklerhandwerk: shall we indeed make only the timestamp parsing experimental?
- @Ericson2314: can we deprecate the builtin altogether?
- @edolstra: no does not make sense write parsers as Nix language libraries
decision:
- keep only the timestamp parsing experimental
- merge

@andreabedini We'd very much welcome a follow up where timestamps are represented as structured Nix values. That would be the precondition to stabilise the feature.

nixos-discourse · 2023-06-09T10:04:29Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-06-05-nix-team-meeting-minutes-60/28933/1

roberth · 2024-09-18T15:27:36Z

An update of the TOML library has changed the returned timestamp's string representation, either to remove the T or to return the original string; needs to be looked into.

eval-okay-fromTOML-timestamps test fails on toml11-4.2.0 #11441

andreabedini requested a review from edolstra as a code owner March 28, 2023 04:52

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Mar 28, 2023

andreabedini mentioned this pull request Mar 28, 2023

Add information about revisions to packages.json input-output-hk/foliage#44

Open

thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Apr 7, 2023

andreabedini force-pushed the toml-timestamps-as-strings branch from 6394e46 to 3eef831 Compare April 21, 2023 08:28

roberth mentioned this pull request Apr 21, 2023

Add builtins.dateFormat function #8163

Open

edolstra reviewed May 8, 2023

View reviewed changes

fricklerhandwerk mentioned this pull request May 9, 2023

fetchTree: add lastModifiedDateISO8601 to sourceInfo #8277

Closed

8 tasks

andreabedini force-pushed the toml-timestamps-as-strings branch from 3eef831 to a20c53a Compare May 22, 2023 08:02

andreabedini force-pushed the toml-timestamps-as-strings branch from a20c53a to b258353 Compare May 25, 2023 01:58

Merge remote-tracking branch 'upstream/master' into toml-timestamps-a…

59fe2f2

…s-strings

fricklerhandwerk reviewed May 31, 2023

View reviewed changes

polykernel mentioned this pull request May 31, 2023

primops: add builtins.toTOML #8416

Closed

8 tasks

fricklerhandwerk self-assigned this Jun 5, 2023

fricklerhandwerk merged commit 3c78920 into NixOS:master Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse TOML timestamps as strings #8120

Parse TOML timestamps as strings #8120

andreabedini commented Mar 28, 2023

Ericson2314 commented Apr 10, 2023 •

edited

Loading

andreabedini commented Apr 12, 2023

nixos-discourse commented Apr 17, 2023

edolstra May 8, 2023 •

edited

Loading

fricklerhandwerk May 8, 2023

fricklerhandwerk commented May 9, 2023

fricklerhandwerk commented May 9, 2023

nixos-discourse commented May 9, 2023

andreabedini commented May 22, 2023

fricklerhandwerk May 31, 2023

andreabedini Jun 1, 2023

fricklerhandwerk commented Jun 9, 2023

nixos-discourse commented Jun 9, 2023

roberth commented Sep 18, 2024

Parse TOML timestamps as strings #8120

Parse TOML timestamps as strings #8120

Conversation

andreabedini commented Mar 28, 2023

Motivation

Context

Checklist for maintainers

Priorities

Ericson2314 commented Apr 10, 2023 • edited Loading

andreabedini commented Apr 12, 2023

nixos-discourse commented Apr 17, 2023

edolstra May 8, 2023 • edited Loading

Choose a reason for hiding this comment

fricklerhandwerk May 8, 2023

Choose a reason for hiding this comment

fricklerhandwerk commented May 9, 2023

fricklerhandwerk commented May 9, 2023

nixos-discourse commented May 9, 2023

andreabedini commented May 22, 2023

fricklerhandwerk May 31, 2023

Choose a reason for hiding this comment

andreabedini Jun 1, 2023

Choose a reason for hiding this comment

fricklerhandwerk commented Jun 9, 2023

nixos-discourse commented Jun 9, 2023

roberth commented Sep 18, 2024

Ericson2314 commented Apr 10, 2023 •

edited

Loading

edolstra May 8, 2023 •

edited

Loading