Skip to content

fetchTree cleanup#9061

Merged
edolstra merged 4 commits intoNixOS:masterfrom
edolstra:stabilize-fetchTree
Oct 13, 2023
Merged

fetchTree cleanup#9061
edolstra merged 4 commits intoNixOS:masterfrom
edolstra:stabilize-fetchTree

Conversation

@edolstra
Copy link
Member

@edolstra edolstra commented Sep 28, 2023

Motivation

Reviewing current builtins.fetchTree behaviour, I made a few changes:

  • The (probably unintentional) hack to handle paths as tarballs has been removed. This is almost certainly not what users expect and is inconsistent with flakeref handling everywhere else.

  • The hack to support scp-style Git URLs has been moved to the Git fetcher, so it's now supported not just by fetchTree but by flake inputs.

Context

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Sep 28, 2023
@edolstra edolstra added this to the Flakes milestone Sep 28, 2023
Ma27

This comment was marked as outdated.

@Ericson2314
Copy link
Member

Ericson2314 commented Sep 28, 2023

I just went and merged #8509 the docs PR (which this includes, and @fricklerhandwerk and approved but did not write) so we can narrow the scope of what's left.

Today we have

 $ git grep -i -l flake src/libfetchers/
src/libfetchers/fetch-settings.hh
src/libfetchers/fetchers.cc
src/libfetchers/fetchers.hh
src/libfetchers/indirect.cc
src/libfetchers/registry.cc
src/libfetchers/tarball.cc

I am curious what does or does still make sense assuming this is stable.

Here is what I recommend:

  • src/libfetchers/registry.{cc,hh} to a new libflakes
  • src/libfetchers/indirect.cc to libflakes too, will need to support experimental InputSchemes to keep that experimental, so we should do so. (Easy, do just like experimental flags, commands, builtins, etc.)
  • src/libfetchers/fetchers.hh only uses "flake" in comments, those comments should be instead reworded to discuss builtins.fetchTree instead; stable code paths exist for stable features and even the comments should reflect that.
  • src/libfetchers/tarball.cc just one comment, I don't fully get it to discuss
  • src/libfetchers/fetch-settings.hh a few settings all of which should be moved further downstream (e.g. the new libflakes or libexpr or libcmd, etc.)

Additionally flakeIdRegex should be moved out of libutil

@edolstra
Copy link
Member Author

I just went and merged #8509 the docs PR

Hm, I didn't merge that PR because there are some things in it that are not correct (like "Fetch a single file from a URL") which this PR fixes.

src/libfetchers/registry.{cc,hh} to a new libflakes

Not in favor of that, it seems like unnecessary overengineering.

Also, considering that fetchGit isn't experimental, isn't that a breaking change?

No, because it widens rather than restricts what fetchGit accepts.

BTW I would be in favor of removing --allowed-uris since there are so many ways to bypass it. It probably gives users a false sense of security.

@edolstra

This comment was marked as outdated.

@Ma27

This comment was marked as outdated.

@roberth

This comment was marked as resolved.

wentasah and others added 4 commits October 13, 2023 14:24
Co-authored-by: Valentin Gagarin <[email protected]>

Supersedes NixOS#6740
Two changes:

* The (probably unintentional) hack to handle paths as tarballs has
  been removed. This is almost certainly not what users expect and is
  inconsistent with flakeref handling everywhere else.

* The hack to support scp-style Git URLs has been moved to the Git
  fetcher, so it's now supported not just by fetchTree but by flake
  inputs.
@edolstra edolstra force-pushed the stabilize-fetchTree branch from 35ddc79 to 8eb4f73 Compare October 13, 2023 12:40
@edolstra edolstra changed the title Stabilize fetchTree fetchTree cleanup Oct 13, 2023
@edolstra edolstra enabled auto-merge October 13, 2023 12:44
@edolstra edolstra disabled auto-merge October 13, 2023 12:46
@edolstra edolstra enabled auto-merge October 13, 2023 12:46
@edolstra edolstra disabled auto-merge October 13, 2023 12:56
@edolstra edolstra enabled auto-merge October 13, 2023 12:56
@edolstra edolstra merged commit 2084312 into NixOS:master Oct 13, 2023
@edolstra edolstra deleted the stabilize-fetchTree branch October 13, 2023 14:44
@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-10-13-nix-team-meeting-minutes-94/34395/1

@angerman
Copy link
Contributor

angerman commented Dec 11, 2023

This commit breaks the following flake:

{
  inputs = {
    nixpkgs.url = "github:nixos/nixpkgs?ref=nixos-unstable";
    flake-utils.url = "github:numtide/flake-utils";
  };
  outputs = { self, nixpkgs, flake-utils }:
    flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = import nixpkgs {
          inherit system;
        };
      in
        {
          devShell = pkgs.mkShell {
            buildInputs = [ pkgs.hello ];
          };
        }
    );
}

with

$ nix clone https://github.com/nixos/nix
$ cd nix
$ git checkout 20843123134cf04d076744c3bf5faf0ab238604f
$ nix run . -- flake show .. --restrict-eval

we now get

error:
       … in the left operand of the update (//) operator

         at «string»:56:13:

           55|             # This is shadowed in the next //
           56|             // sourceInfo
             |             ^
           57|             // {

       … in the condition of the assert statement

         at «string»:66:13:

           65|           if node.flake or true then
           66|             assert builtins.isFunction flake.outputs;
             |             ^
           67|             result

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: access to URI 'github:numtide/flake-utils/4022d587cbbfd70fe950c1e2083a02621806a725' is forbidden in restricted mode

and all my flakes in hydra are now broken 😕

@roberth
Copy link
Member

roberth commented Dec 11, 2023

Fixable with #9547 and then and adding the desired schemes to allowed-uris.

@angerman
Copy link
Contributor

Thanks, so I basically now need to explicitly list github:... instead of the https:// from before.

@roberth
Copy link
Member

roberth commented Dec 11, 2023

now

After #9547, yes, but I haven't confirmed that https:// would not be necessary.

@angerman
Copy link
Contributor

I'm currently having something like this:

nix.extraOptions = let orgs = [
            "NixOS"
            ...and.many.more...
        ]; in ''
        allowed-uris = github: ${__concatStringsSep " " (map (org: "github:${org} https://github.com/${org}") orgs)}
        '';

🤮

@bryango
Copy link
Member

bryango commented Jan 10, 2024

I believe since this PR the flake ref git+file:./${submodule} no longer works; see:

Is there any alternative way to specify the flake input of a git submodule in a subdirectory?

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

Labels

documentation fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command 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.

9 participants