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

fetchTree: add lastModifiedDateISO8601 to sourceInfo #8277

Closed
wants to merge 1 commit into from

Conversation

Kranzes
Copy link
Member

@Kranzes Kranzes commented Apr 30, 2023

Motivation

I thought it would make sense to have fetchTree's sourceInfo provide a variant of lastModifiedDate in the ISO 8601 format that is already used many times across nixpkgs for packages with a version of unstable-YYYY-MM-DD.

Context

#8163

NixOS/nixpkgs#228802 (comment) (Initially made it as a nixpkgs lib but @infinisil suggested to make it in Nix itself)

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
  • documentation in the internal API docs
  • 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.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Apr 30, 2023
@Kranzes Kranzes force-pushed the master branch 2 times, most recently from f5acb16 to f7b15be Compare April 30, 2023 15:38
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented May 9, 2023

Reviewed in Nix team meeting 2023-05-08:

We decided to close this PR, since the suggested transformation is trivial and we think it should be done on the consumer side for the sake of keeping the interface minimal. The heavy lifting is already done (also implemented in the Nix language in flake-compat). The rationale for having a string without separators is so it can be used in version identifiers easily.

That may not be the optimal design overall, and given flakes are still experimental it could in principle be changed. We spent some time considering how much support Nix should provide for handling time-related data in general, since it's quite involved when one needs to get it right. Given we don't have evidence of clear need, and no pull requests have been made in that direction, this topic has no priority right now. Nonetheless we welcome design proposals.

Complete discussion
  • @edolstra: the transformation is trivial and for the sake of a minimal interface should be done on the consumer side
  • @roberth: the original transformation from Unix timestamp is also implemented in flake-compat
  • @fricklerhandwerk: why is lastModifiedDate not represented in ISO 8601 to begin with?
    • @edolstra: because it can be used as a version component without further transformation
  • @Ericson2314: what about having a structured record so we can avoid parsing altogether?
    • @edolstra: would be more inclined to add something like that
      • this is a more general issue: ideally Nixpkgs would have some standardised record with specified meaning so Nix doesn't have to come up with custom formats
      • @fricklerhandwerk: would prefer answering the general question
        • @edolstra: it's a non-trivial design problem
          • could provide a builtin for parsing ISO dates
          • it could be done in Nixpkgs, but the Nix language is not the greatest tool for the job
          • C++ has libraries for that, but they may be platform dependent
          • @roberth: Nixpkgs has all the infrastructure and a maintainers community who can take the responsibility
            • @fricklerhandwerk: driven to conclusion that would mean that we should remove lastModifiedDate and leave that to consumers entirely
              • @roberth: agreed. the attribute was a sensible choice historically, but given this is an experimental feature we still have a choice
              • @edolstra: don't agree. it would inflict a lot of pain on users, and we're actually aiming to stabilise flakes. at best it would remove two lines from Nix
            • @edolstra: it may become a performance issue for consumers like haskell.nix
              • @roberth: this would speak for a builtin, but we shouldn't rush it.
    • @roberth: related: Parse TOML timestamps as strings #8120
  • @fricklerhandwerk: how important is a proper time representation? is it vital or more of a surface feature?
    • @edolstra: it's mostly used for version strings. there is no structured representation of time because it wasn't really needed yet.
    • @roberth: hard to tell for Nixpkgs. seems not very relevant, but that may be because people don't use it due to lack of proper support.
      • the odds of needing a full-blown time library are small.
      • @edolstra: for systemd it would be nice to have a structured way of specifying intervals. but it's a different use case than a datetime specification
        • @fricklerhandwerk: yes, there are even more data types one would have to consider to cover time handling
  • decision: closed

@nixos-discourse
Copy link

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

@Kranzes
Copy link
Member Author

Kranzes commented May 9, 2023

I agree with the decisions being made, I wil still try to get some function for this in nixpkgs. While you're at it, consider closing this as well #8163.

@fricklerhandwerk
Copy link
Contributor

consider closing this as well #8163

We can use this for tracking the time handling question. No clear reason to close it yet. The precise problem statement can still be adapted.

@Kranzes
Copy link
Member Author

Kranzes commented May 9, 2023

We can use this for tracking the time handling question. No clear reason to close it yet. The precise problem statement can still be adapted.

Maybe comment on there as well, because technically I implemented exactly what that issue wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants