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

Allow Flake inputs to accept boolean and integer attributes #4435

Merged
merged 3 commits into from
Jan 11, 2021

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Jan 8, 2021

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):

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.
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) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably go for a switch here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a commit that switches this to a switch.

@knedlsepp
Copy link
Member

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

src/libexpr/flake/flake.cc Outdated Show resolved Hide resolved
@DanilaFe
Copy link
Contributor Author

DanilaFe commented Jan 9, 2021

@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.

@L-as
Copy link
Member

L-as commented Mar 1, 2021

@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
Copy link

nrdxp commented Mar 11, 2021

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
Copy link

I am also getting submodule is not a Boolean.

@mhuesch
Copy link

mhuesch commented Nov 27, 2021

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.

mhuesch added a commit to neighbour-hoods/social_sensemaker that referenced this pull request Nov 27, 2021
`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
Copy link

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
Copy link

nrdxp commented Oct 26, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flake inputs: git submodules/recursive checkout
9 participants