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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parseFlakeRef and flakeRefToString builtins #8670

Merged
merged 20 commits into from
Jul 25, 2023

Conversation

aakropotkin
Copy link
Contributor

@aakropotkin aakropotkin commented Jul 8, 2023

Motivation

Over the last year or so I've run into several use cases where I need to parse and/or serialize URLs for use by builtins.fetchTree or builtins.getFlake, largely in order to produce lockfile-like files for lang2nix frameworks or tools which use nix internally to drive builds.

I've gone through the painstaking process of emulating nix::FlakeRef::fromAttrs and nix::parseFlakeRef several times with mixed success; but these are difficult to create and even harder to maintain if I hope to stay aligned with changes to the real parser/serializer.

Context

I understand why adding new builtins isn't something we want to do flagrantly. I'm recommending this addition simply because I keep encountering use cases where I need to parse/serialize these URIs in nix expressions, and I want a reliable solution.

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 the with-tests Issues related to testing. PRs with tests have some priority label Jul 9, 2023
@aakropotkin aakropotkin changed the title add parseFlakeRef builtin add parseFlakeRef and flakeRefToString builtins Jul 9, 2023
@aakropotkin aakropotkin marked this pull request as ready for review July 9, 2023 13:40
@aakropotkin
Copy link
Contributor Author

@edolstra bump 鉂わ笍

@thufschmitt thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jul 24, 2023
@Ericson2314 Ericson2314 changed the title add parseFlakeRef and flakeRefToString builtins add parseFlakeRef and flakeRefToString builtins Jul 24, 2023
src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved
aakropotkin and others added 7 commits July 24, 2023 06:55
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

With those docs changes
(we don't really worry about line length in the multiline strings, but rather try to use 1 sentance per line.)
and the release notes conflicts fixed (because 2.17 was just cut) this is good!

aakropotkin and others added 2 commits July 25, 2023 07:50
Co-authored-by: John Ericson <git@JohnEricson.me>
Co-authored-by: John Ericson <git@JohnEricson.me>
@Ericson2314 Ericson2314 changed the title add parseFlakeRef and flakeRefToString builtins Add parseFlakeRef and flakeRefToString builtins Jul 25, 2023
@Ericson2314
Copy link
Member

note the release notes still have a conflict, because 2.17 was just released.

@aakropotkin
Copy link
Contributor Author

note the release notes still have a conflict, because 2.17 was just released.

Got that conflict resolve and merged the provided doc changes ( thanks for tweaking those ).

@Ericson2314 Ericson2314 enabled auto-merge (squash) July 25, 2023 16:45
@Ericson2314 Ericson2314 merged commit 2d1d811 into NixOS:master Jul 25, 2023
8 checks passed
@thufschmitt
Copy link
Member

Coming a bit late, but from the 2023-07-24 Nix maintainers meeting:

Approved on the principle, assigned to @Ericson2314

@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-07-24-nix-team-meeting-minutes-74/31116/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants