Combined parsing and schema generations (libfetchers, WIP) #9273

roberth · 2023-11-01T23:16:18Z

Motivation

By combining parsing and schema generation (for docs), we achieve the following

keep the implementation and docs closely in sync
make it easy to have fields for every attribute
- this will have a canonicalizing effect on the output of the fetcher. Improves consistency of returned values (and perhaps default handling)

Eventually, this helps with removing submodules "leak"

TODO

It's currently not useful at all.

Must have

resolve the little conflict
add documentation fields
write the Schema to JSON
union (e.g. path or string)

Should have

Input subclasses,
- or at least move checks into the lambda where parsed values are directly available)
type tag node in the schema

Nice to have

add libexpr-level schema check, as that will be able to refer to attribute positions

Context

Supplements libfetcher doc automation #9255

Priorities

Add 👍 to pull requests you find important.

Ericson2314 · 2023-11-01T23:26:52Z

src/libfetchers/tests/parser.cc

+using namespace testing;
+
+namespace nix::fetchers {
+    using namespace parsers;


I think we should deindent these like the rest of the code (and go back and deindent the other indented tests too.

Ericson2314 · 2023-11-01T23:28:16Z

src/libfetchers/schema.hh

+            bool required;
+            std::shared_ptr<Schema> type;
+            bool operator==(const Attr & other) const;
+        };


Suggested change

};

};

Ericson2314 · 2023-11-01T23:28:29Z

src/libfetchers/schema.hh

+        Attrs() {};
+        Attrs(std::map<std::string, Attr> && attrs)
+            : attrs(attrs) {};
+    };


Suggested change

};

};

Ericson2314 · 2023-11-01T23:28:51Z

src/libfetchers/attrs.cc

@@ -5,6 +5,14 @@

 namespace nix::fetchers {

+std::string attrType(const Attr &attr) {


Suggested change

std::string attrType(const Attr &attr) {

std::string attrType(const Attr & attr) {

our usual format

Ericson2314 · 2023-11-01T23:30:42Z

src/libfetchers/attrs.hh

@@ -12,6 +12,9 @@

 namespace nix::fetchers {

+/**
+ * A primitive value that can be used in a fetcher attribute.
+ */
 typedef std::variant<std::string, uint64_t, Explicit<bool>> Attr;


In some later PR we should probably do a wrapper struct (per #7479 (comment)) so the functions below can be methods.

Ericson2314 · 2023-11-01T23:32:41Z

src/libfetchers/parser.hh

+
+namespace nix::fetchers {
+
+    struct Schema;


I would not indent the outermost namespace

Suggested change

struct Schema;

struct Schema;

Ericson2314 · 2023-11-01T23:34:31Z

src/libfetchers/parser.hh

+            std::apply([&](auto&&... args) { (f(args), ...); }, t);
+        }
+
+        /** Accepts an 'Attrs'. Composes 'Attr' (singular) parsers. */


Suggested change

/** Accepts an 'Attrs'. Composes 'Attr' (singular) parsers. */

/** Accepts an `Attrs`. Composes `Attr` (singular) parsers. */

Ericson2314 · 2023-11-01T23:38:51Z

src/libfetchers/parser.hh

+
+        public:
+            Attrs(Callable lambda, AttrParsers *... parsers)
+                : lambda(lambda), parsers(parsers...) {


Suggested change

: lambda(lambda), parsers(parsers...) {

: lambda(lambda), parsers(parsers...)

{

Ericson2314 · 2023-11-01T23:39:07Z

src/libfetchers/parser.hh

+        class Attrs
+            : public Parser<
+                nix::fetchers::Attrs,
+                std::invoke_result_t<Callable, typename AttrParsers::Out ...>> {


Suggested change

std::invoke_result_t<Callable, typename AttrParsers::Out ...>> {

std::invoke_result_t<Callable, typename AttrParsers::Out ...>>

{

nixos-discourse · 2023-11-09T16:20:39Z

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

https://discourse.nixos.org/t/2023-11-03-nix-team-meeting-minutes-100/35245/1

We already have some maybeGet*Attr functions - just not one for std::map itself yet.

Name will be used in upcoming commit.

roberth · 2024-01-09T22:08:22Z

src/libfetchers/tarball.cc

+        new OptionalAttr<TarballAttrs,Int>("revCount", Int {}, [](auto x) { return x.revCount; }),
+        new OptionalAttr<TarballAttrs,Int>("lastModified", Int {}, [](auto x) { return x.lastModified; })
+    );
+}();


I'm debating whether this is a good solution.
Good:

it's functional

we can initialize a const TarballAttrs

no lists of functions or such, that may slow it down (as it would be nice to have a similar solution that works for defining primops with attr based arguments etc)

bidirectional

Bad:

boilerplate

too little type inference

unparseAttr is super ad hoc. Doesn't have a proper interface. Just "happy" template accidents.

making the arguments and parser codecs line up is a manual process and the error messages are useless

Perhaps a Setting-like alternative would be easier to use?
It would allow us to spend some template "budget" on declaring what kinds of fields they are, so that we have a variation of the TarballAttrs to represent a properly locked input, or a variation for the attrs that should be returned by fetchTree.

Maybe a syntax like this can provide better inference, at the cost of needing another type.

OptionalAttr("revCount", Int {}).getter([](TarballAttrs x){ return x.revCount; })

Regarding the boilerplate - maybe feel quite that way if we make more use of this feature when extended with doc strings.

The renaming task is splatting everything together into markdown lists. I think I want to do that with the lowdown AST; I opened kristapsdz/lowdown#131 for this purpose PR #9273 should be able to improve upon this a good bit, but I think it is still a useful stepping stone.

The renaming task is splatting everything together into markdown lists. I think I want to do that with the lowdown AST; I opened kristapsdz/lowdown#131 for this purpose PR NixOS#9273 should be able to improve upon this a good bit, but I think it is still a useful stepping stone.

The renaming task is splatting everything together into markdown lists. I think I want to do that with the lowdown AST; I opened kristapsdz/lowdown#131 for this purpose PR #9273 should be able to improve upon this a good bit, but I think it is still a useful stepping stone.

github-actions bot added with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Nov 1, 2023

Ericson2314 reviewed Nov 1, 2023

View reviewed changes

src/libfetchers/schema.hh

Attrs() {};

Attrs(std::map<std::string, Attr> && attrs)

: attrs(attrs) {};

};

Copy link

Member

Ericson2314 Nov 1, 2023

Choose a reason for hiding this comment

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

Suggested change

};

};

Ericson2314 reviewed Nov 1, 2023

View reviewed changes

roberth mentioned this pull request Nov 10, 2023

Introduce libgit2 #9240

Merged

roberth mentioned this pull request Dec 10, 2023

Make libfetchers Input an interface with virtual methods. #9578

Open

roberth added 12 commits January 9, 2024 00:34

Add nix::fetchers::attrType

6700924

Add nix::fetchers::parsers::String

52a5328

Add nix::fetchers::parsers::{Bool,Int}

e3e8771

Add nix::fetchers::Schema::Attrs

9cf0927

libutil: Add maybeGet to return optional from map lookup

6f528f9

We already have some maybeGet*Attr functions - just not one for std::map itself yet.

nix::fetchers::parsers: Make Attr fully qualified

cf3bc19

Name will be used in upcoming commit.

Add convenience initializer to fetchers::Schema::Attrs

f571cb3

Add nix::fetchers::parsers::Attrs etc

c25db0c

Add const to Parser::{parse,getSchema}

2f8f456

refact: Move src/libfetchers/tests to tests/unit/libfetchers

eebdec5

Fix compilation error

7fef87c

WIP make it bidirectional and use for type = tarball

f014413

roberth force-pushed the libfetchers-combined-parsing-schema branch from 9c75cb8 to f014413 Compare January 9, 2024 19:08

roberth commented Jan 9, 2024

View reviewed changes

Ericson2314 mentioned this pull request Jan 9, 2024

Systematize builtins.fetchTree docs #9732

Open

fricklerhandwerk mentioned this pull request Mar 31, 2024

Declarative signature specification for built-in functions #10373

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combined parsing and schema generations (libfetchers, WIP) #9273

Combined parsing and schema generations (libfetchers, WIP) #9273

roberth commented Nov 1, 2023 •

edited

Loading

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023 •

edited

Loading

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023

Ericson2314 Nov 1, 2023

nixos-discourse commented Nov 9, 2023

roberth Jan 9, 2024

roberth Jan 9, 2024

		@@ -5,6 +5,14 @@

		namespace nix::fetchers {

		std::string attrType(const Attr &attr) {

	std::string attrType(const Attr &attr) {
	std::string attrType(const Attr & attr) {

	/** Accepts an 'Attrs'. Composes 'Attr' (singular) parsers. */
	/** Accepts an `Attrs`. Composes `Attr` (singular) parsers. */

	: lambda(lambda), parsers(parsers...) {
	: lambda(lambda), parsers(parsers...)
	{

	std::invoke_result_t<Callable, typename AttrParsers::Out ...>> {
	std::invoke_result_t<Callable, typename AttrParsers::Out ...>>
	{

Combined parsing and schema generations (libfetchers, WIP) #9273

Are you sure you want to change the base?

Combined parsing and schema generations (libfetchers, WIP) #9273

Conversation

roberth commented Nov 1, 2023 • edited Loading

Motivation

TODO

Context

Priorities

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Nov 1, 2023 •

edited

Loading

Ericson2314 Nov 1, 2023 •

edited

Loading