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

Poor fetchTree documentation #9249

Closed
2 tasks
infinisil opened this issue Oct 27, 2023 · 7 comments
Closed
2 tasks

Poor fetchTree documentation #9249

infinisil opened this issue Oct 27, 2023 · 7 comments
Assignees

Comments

@infinisil
Copy link
Member

Problem

Now that fetchTree has been partially stabilised, I wanted to check the docs to see what is supported, but the docs turn out to be very poor:

The entry point builtins.fetchTree only has this sentence on what argument it takes:

Fetch a source tree or a plain file using one of the supported backends. input can be an attribute set representation of flake reference or a URL.

The fetchTree stabilisation PR further adds to this that the URL-syntax requires flakes to be enabled, okay so only the attribute set representation is supported.

So going to the flake references documentation, the attribute set syntax is documented as follows:

Example:

{
  type = "github";
  owner = "NixOS";
  repo = "nixpkgs";
}

The only required attribute is type. The supported types are listed below.

The rest of the document just documents the URL-syntax!

Furthermore, the Types section (which is quite a bunch further down and isn't linked to) really doesn't clear things up:

path: arbitrary local directories, or local Git trees. The required attribute path specifies the path of the flake. The URL form is

[path:]<path>(\?<params)?

where path is an absolute path.

For one, the path type is not yet stable, but it's not even mentioned here (or anywhere else for that matter). Furthermore, it's really unclear what the attribute set syntax is supposed to be for this type..

{
  type = "path";
  path = "...";
  # Is this a supported attribute?
  params = "??";
}

Let's check out the now-stable git type:

git: Git repositories. The location of the repository is specified by the attribute url.

They have the URL form

git(+http|+https|+ssh|+git|+file|):(//<server>)?<path>(\?<params>)?

Immediate questions that aren't answered even reading further:

  • Is server, path, params a supported attribute?
  • How does url relate to this long line? I guess probably everything after the (optional) + could be the URL
  • What is git+git supposed to be? And generally what do all of these + syntax do? There's only an explanation for git+file
  • How can I express this + syntax with an attribute set? type = "git+path" doesn't seem to work

Furthermore, the section on flake reference attributes doesn't help:

  • Apparently dir is a generic attribute supported by all types, but if you read one sentence of it, it's doing something specific to a flake.nix. And trying it out it doesn't work..
    nix-repl> builtins.fetchTree { type = "git"; url = ./.; dir = "source"; }
    error: unsupported Git input attribute 'dir'
    
  • Okay so narHash is a generic one, but trying it out, it seems to change the output attributes!
    nix-repl> builtins.fetchTree { type = "git"; url = ./.; }                                             
    { lastModified = 1698359940; lastModifiedDate = "20231026223900"; narHash = "sha256-GqDocJl7bb7g2cu+sw1VAd6ld6G3054kbmwm0QfDhfQ="; outPath = "/nix/store/l4ixyl93hdv30ni76m0058a23rfis2im-source"; rev = "a6e587923c9d5d716fe0f0049bed96d1cc210bff"; revCount = 15232; shortRev = "a6e5879"; submodules = false; }
    
    nix-repl> builtins.fetchTree { type = "git"; url = ./.; narHash = "sha256-GqDocJl7bb7g2cu+sw1VAd6ld6G3054kbmwm0QfDhfQ="; }
    { narHash = "sha256-GqDocJl7bb7g2cu+sw1VAd6ld6G3054kbmwm0QfDhfQ="; outPath = "/nix/store/l4ixyl93hdv30ni76m0058a23rfis2im-source"; submodules = false; }
    

More generally, it's not documented what builtins.fetchTree even returns! The only clue we have is an example in the builtin docs, but the above shows that it's not general

Reading further in the flake reference attribute section:

Finally, some attribute are typically not specified by the user, but can occur in locked flake references and are available to Nix code:

It then lists revCount and lastModified, but it's unclear what "available to Nix code means". From the above evaluation these seem to be output attributes, or does "available" also mean they can be specified in the input?

nix-repl> builtins.fetchTree { type = "git"; url = ./.; narHash = "sha256-GqDocJl7bb7g2cu+sw1VAd6ld6G3
054kbmwm0QfDhfQ="; revCount = 15233; }                    
{ narHash = "sha256-GqDocJl7bb7g2cu+sw1VAd6ld6G3054kbmwm0QfDhfQ="; outPath = "/nix/store/l4ixyl93hdv30ni76m0058a23rfis2im-source"; revCount = 15233; submodules = false; }

Yup that seems to work, I can specify revCount and it just seems to get passed through without checking, the revCount is actually wrong here. What if I don't pass the narHash anymore?

nix-repl> builtins.fetchTree { type = "git"; url = ./.; revCount = 15233; } 
error: 'revCount' attribute mismatch in input 'git+file:///home/tweagysil/src/nix?ref=refs/heads/master&rev=a6e587923c9d5d716fe0f0049bed96d1cc210bff', expected 15233

Oh only then it complains about it being wrong. What about shortRev though? That was there in the output without a narHash, was that just not documented but it works the same as revCount?

nix-repl> builtins.fetchTree { type = "git"; url = ./.; shortRev = "a6e5870"; }
error: unsupported Git input attribute 'shortRev'

No that is not supported at all in the input.

So in conclusion, the docs for fetchTree really suck.

Proposal

Clean up this mess before calling fetchTree stable. I have really no clue what exactly is really supposed to be stable now.

Checklist

Priorities

Add 👍 to issues you find important.

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 27, 2023

Thank you for this exhaustive look over the docs, @infinisil. I would add we really shouldn't be stabilizing anything at all until it is properly documented (and tested); that would constitute a regression in our quality (and test coverage).

I think this situation needs to be fixed by the Nix release (when it could actually become stable), or else builtins.fetchTree should be made unstable again.

I understand this might feel like a hoop to jump through for those that think that the real benefit is Flakes, builtins.fetchTree is just some bullshit along the way that others have imposed. But for those of us that subscribe to the "flakes is doing too much at us", builtins.fetchTree is not just some bullshit along the way; it needs to be high quality; Flakes needs to be decomposed into real other features, not pretend things we don't intend anyone to use.

@Ericson2314
Copy link
Member

Because we have the pluggable fetchers arch, I would expect that incorporating it into the CLI dump the way we do with builtins, flags, etc. might be the right way to go. Ideally we can make some of reference docs of individual fields etc. more automated and easy to keep up to date that way.

@fricklerhandwerk
Copy link
Contributor

In any case, I'll soon start working on the docs as promised so we have something written down at all. @infinisil thanks a lot for the compilation.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-10-27-nix-team-meeting-minutes-98/34695/1

@infinisil
Copy link
Member Author

This PR improves the docs: #9258

@roberth
Copy link
Member

roberth commented Dec 1, 2023

I've made an attempt to make sense of how the fetchers and flakes features interact.
It's not a particularly well-structured graph, which is unfortunate, but it might be an idea or starting point for something.

flowchart TD

classDef process fill:#9f6
classDef world fill:#f7f7f7,stroke:none

world([world]):::world 
ref([source ref])
lock([lock])


%% e.g. revCount
metadata([metadata])

%% things like git tree hash if you already have a commit hash.
%% cacheable things that may speed up the fetch
optional([optional lock attrs])
contents([store contents])

ref--> locking
world --> locking
world([the world])

locking:::process --> lock
locking --> metadata
locking -.-> cache
locking -.-> optional

flake[call-flake]:::process

cache([cache])
cache --> fetchToStore

lock --> flake
metadata --> flake

optional -.-> fetchToStore

outPath([outPath])

sourceInfo([sourceInfo])

flake --> sourceInfo


lock --> fetchToStore:::process
world --> fetchToStore
fetchToStore --> contents
fetchToStore --> outPath

strip[strip]
strip:::process --> ref
flakeref([flakeref])
flakeref --> strip

outPath --> flake

subgraph fetchers
cache
locking
fetchToStore
metadata
lock
optional
end

Loading

@infinisil
Copy link
Member Author

I consider this mostly addressed now that #9258 is merged

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

No branches or pull requests

5 participants