Allow Flake inputs to accept boolean and integer attributes #4435

DanilaFe · 2021-01-08T02:53:15Z

Looking through the code base for Nix, I found that it creates fetchers::Input from the attributes given to Flakes' input refs (see also lines 108 through 133 in flake.cc):

nix/src/libexpr/flake/flakeref.cc

Lines 191 to 198 in 920e6a6

    
           FlakeRef FlakeRef::fromAttrs(const fetchers::Attrs & attrs) 
        
           { 
        
               auto attrs2(attrs); 
        
               attrs2.erase("dir"); 
        
               return FlakeRef( 
        
                   fetchers::Input::fromAttrs(std::move(attrs2)), 
        
                   fetchers::maybeGetStrAttr(attrs, "dir").value_or("")); 
        
           }

The attributes accepted by the fetches can be strings, integers, or booleans. However, currently, the code in flake.cc rejects all attributes that are not strings. It does, however, forward all extra string arguments, no matter what they are, to the fetchers::Input. This seems inconsistent, and prevents input types like Git from accepting certain arguments, including submodules.

This PR expands the code in flake.cc to accept non-string arguments. Combined with this, I was able to make non-flake inputs with recursive git modules work as follows:

inputs = {
    blog-source = {
      flake = false;
      url = "https://my.example.com/repo.git";
      type = "git";
      submodules = true;
    }
};

Note that type = "git" is already supported by Nix; this PR allows the submodules = true argument to be passed in alongside type. I believe this would close #4357 (and its duplicate, #4423).

So far, the only non-integer attributes are submodules (bool), revCount (int), and lastModified (int). While the latter two are not particularly useful, I think the benefits for git submodules and consistency make this a worthwhile change.

I believe that this makes it possible to do things like Git inputs with submodules, but it also likely applies to other input types from libfetchers.

infinisil · 2021-01-08T02:57:08Z

src/libexpr/flake/flake.cc

-                    throw TypeError("flake input attribute '%s' is %s while a string is expected",
+                } else if (attr.value->type() == nBool) {
+                    attrs.emplace(attr.name, Explicit<bool>{ attr.value->boolean });
+                } else if (attr.value->type() == nInt) {


I'd probably go for a switch here

I didn't realize it was an enum! Yes, switch works much better here.

I pushed a commit that switches this to a switch.

knedlsepp · 2021-01-08T06:46:16Z

Do you know how one would make sure that nix fetches submodules for self with this?

src/libexpr/flake/flake.cc

DanilaFe · 2021-01-09T00:10:42Z

@knedlsepp unfortunately, I'm not sure. This change affects only the inputs, which self is not part of. So, to make sure a flake has its submodules initialized and cloned, it must be that whatever is invoking the flake has submodules enabled. With this change, it's impossible to force the current flake to have its own submodules resolved, only its dependencies. I think this is still consistent, though - a flake's own flake.nix does not have control over the fetcher used to retrieve it, as far as I know.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

L-as · 2021-03-01T20:52:09Z

@knedlsepp unfortunately, I'm not sure. This change affects only the inputs, which self is not part of. So, to make sure a flake has its submodules initialized and cloned, it must be that whatever is invoking the flake has submodules enabled. With this change, it's impossible to force the current flake to have its own submodules resolved, only its dependencies. I think this is still consistent, though - a flake's own flake.nix does not have control over the fetcher used to retrieve it, as far as I know.

What you say makes sense, but when you consider how it's actually used, it's very inconvenient. A flake will either always want its submodules initialized, or never want to. A flake that has submodules will not work if you don't enable them, so there is no point in allowing a user to not fetch the submodules for a flake (if they are required to build it).

Since whether submodules should be fetched depends on the flake, I propose that the flake set for itself whether its submodules should be fetched. You could also just unconditionally fetch them for flakes, since I can't see any scenario where you wouldn't want them. It's not foreign repositories being fetched for nixpkgs; they're flakes fit for nix. If the owner has added a submodule, it's likely required for using the flake.

Would a PR enabling fetching of submodules unconditionally for all flakes be accepted?

nrdxp · 2021-03-11T04:36:00Z

I'm getting a very strange error when trying to use this. After successfully fetching the module and all submodules, nix gave an error that disallowed usage saying:

submodule is not a Boolean

when it fact it is. I was able to work around by manually removing the submodule keys from the flake.lock and it works without it, since the repo has already been fetched.

hurricanehrndz · 2021-04-13T02:49:45Z

I am also getting submodule is not a Boolean.

mhuesch · 2021-11-27T09:06:32Z

I am running into the same issue that @L-as nicely described, where my self flake has submodules and I am stuck in the inconvenient situation of not being able to fetch them.

I would be interested in contributing to a flake-submodule-fetcher PR if such a thing would be accepted.

`self` flake can't have submodules. see: NixOS/nix#5312 NixOS/nix#4435 for now, it seems cleaner to just work through `Cargo.toml`, with specified git revisions...

kenranunderscore · 2022-03-18T09:46:02Z

I'm also in the same situation as @mhuesch and @L-as.
Here is a post by someone showing how to correct this when invoking from the command line. Is there an update on how to fetch submodules for self? As prefixing every nix build target with .?submodules=1 seems quite inconvenient for me.

Ofc I could try converting the submodule to a flake input, but that creates different inconveniences for us.

nrdxp · 2022-10-26T19:56:05Z

I could try converting the submodule to a flake input, but that creates different inconveniences for us.

It would be inconvenient to do manually, but if Nix itself could gain the power of locking the submodules as flake inputs so that Nix can handle the fetching of them lazily, that might be the best solution in the long run.

Allow Flake inputs to accept boolean and integer attributes

93f1678

I believe that this makes it possible to do things like Git inputs with submodules, but it also likely applies to other input types from libfetchers.

infinisil reviewed Jan 8, 2021

View reviewed changes

Use switch statement instead of sequence of ifs

ba0f841

edolstra reviewed Jan 8, 2021

View reviewed changes

src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved

Upcase "Boolean" in Flake attribute type error

1db3f84

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

edolstra merged commit 7480f2b into NixOS:master Jan 11, 2021

SomeoneSerge mentioned this pull request Feb 17, 2021

Input boolean arguments might be being ignored/git submodules edolstra/flake-compat#20

Closed

ldesgoui mentioned this pull request Aug 1, 2021

Write Nix 2.4 release notes #4521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Flake inputs to accept boolean and integer attributes #4435

Allow Flake inputs to accept boolean and integer attributes #4435

DanilaFe commented Jan 8, 2021

infinisil Jan 8, 2021

DanilaFe Jan 8, 2021

DanilaFe Jan 8, 2021

knedlsepp commented Jan 8, 2021

DanilaFe commented Jan 9, 2021

L-as commented Mar 1, 2021

nrdxp commented Mar 11, 2021

hurricanehrndz commented Apr 13, 2021

mhuesch commented Nov 27, 2021

kenranunderscore commented Mar 18, 2022

nrdxp commented Oct 26, 2022

	FlakeRef FlakeRef::fromAttrs(const fetchers::Attrs & attrs)
	{
	auto attrs2(attrs);
	attrs2.erase("dir");
	return FlakeRef(
	fetchers::Input::fromAttrs(std::move(attrs2)),
	fetchers::maybeGetStrAttr(attrs, "dir").value_or(""));
	}

Allow Flake inputs to accept boolean and integer attributes #4435

Allow Flake inputs to accept boolean and integer attributes #4435

Conversation

DanilaFe commented Jan 8, 2021

infinisil Jan 8, 2021

Choose a reason for hiding this comment

DanilaFe Jan 8, 2021

Choose a reason for hiding this comment

DanilaFe Jan 8, 2021

Choose a reason for hiding this comment

knedlsepp commented Jan 8, 2021

DanilaFe commented Jan 9, 2021

L-as commented Mar 1, 2021

nrdxp commented Mar 11, 2021

hurricanehrndz commented Apr 13, 2021

mhuesch commented Nov 27, 2021

kenranunderscore commented Mar 18, 2022

nrdxp commented Oct 26, 2022