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

Merge in latest master #2

Open
wants to merge 2,756 commits into
base: ipfs_binary_cache
Choose a base branch
from

Conversation

Ericson2314
Copy link
Collaborator

It builds, but I need to figure out how to test this.

bburdette and others added 30 commits April 24, 2020 21:40
Reduces the number of store queries it performs. Also prints a warning
if any of the selectors did not match any installed derivations.

UX Caveats:
- Will print a warning that nothing matched if a previous selector
  already removed the path
- Will not do anything if no selectors were provided (no change from
  before).

Fixes NixOS#3531
Suppose I have a path /nix/store/[hash]-[name]/a/a/a/a/a/[...]/a,
long enough that everything after "/nix/store/" is longer than 4096
(MAX_PATH) bytes.

Nix will happily allow such a path to be inserted into the store,
because it doesn't look at all the nested structure.  It just cares
about the /nix/store/[hash]-[name] part.  But, when the path is deleted,
we encounter a problem.  Nix will move the path to /nix/store/trash, but
then when it's trying to recursively delete the trash directory, it will
at some point try to unlink
/nix/store/trash/[hash]-[name]/a/a/a/a/a/[...]/a.  This will fail,
because the path is too long.  After this has failed, any store deletion
operation will never work again, because Nix needs to delete the trash
directory before recreating it to move new things to it.  (I assume this
is because otherwise a path being deleted could already exist in the
trash, and then moving it would fail.)

This means that if I can trick somebody into just fetching a tarball
containing a path of the right length, they won't be able to delete
store paths or garbage collect ever again, until the offending path is
manually removed from /nix/store/trash.  (And even fixing this manually
is quite difficult if you don't understand the issue, because the
absolute path that Nix says it failed to remove is also too long for
rm(1).)

This patch fixes the issue by making Nix's recursive delete operation
use unlinkat(2).  This function takes a relative path and a directory
file descriptor.  We ensure that the relative path is always just the
name of the directory entry, and therefore its length will never exceed
255 bytes.  This means that it will never even come close to AX_PATH,
and Nix will therefore be able to handle removing arbitrarily deep
directory hierachies.

Since the directory file descriptor is used for recursion after being
used in readDirectory, I made a variant of readDirectory that takes an
already open directory stream, to avoid the directory being opened
multiple times.  As we have seen from this issue, the less we have to
interact with paths, the better, and so it's good to reuse file
descriptors where possible.

I left _deletePath as succeeding even if the parent directory doesn't
exist, even though that feels wrong to me, because without that early
return, the linux-sandbox test failed.

Reported-by: Alyssa Ross <[email protected]>
Thanks-to: Puck Meerburg <[email protected]>
Tested-by: Puck Meerburg <[email protected]>
Reviewed-by: Puck Meerburg <[email protected]>
Fix long paths permanently breaking GC
Set GCROOT to store path to prevent garbage collection
This closes NixOS#3026 by allowing `builtins.readFile` to read a file with a
wrongly reported file size, for example, files in `/proc` may report a
file size of 0. Reading file in `/proc` is not a good enough motivation,
however I do think it just makes nix more robust by allowing more file
to be read.  Especially, I do considerer the previous behavior to be
dangerous because nix was previously reading truncated files. Examples
of file system which incorrectly report file size may be network file
system or dynamic file system (for performance reason, a dynamic file
system such as FUSE may generate the content of the file on demand).

```
nix-repl> builtins.readFile "/proc/version"
""
```

With this commit:

```
nix-repl> builtins.readFile "/proc/version"
"Linux version 5.6.7 (nixbld@localhost) (gcc version 9.3.0 (GCC)) #1-NixOS SMP Thu Apr 23 08:38:27 UTC 2020\n"
```

Here is a summary of the behavior changes:

- If the reported size is smaller, previous implementation
was silently returning a truncated file content. The new implementation
is returning the correct file content.

- If a file had a bigger reported file size, previous implementation was
failing with an exception, but the new implementation is returning the
correct file content. This change of behavior is coherent with this pull
request.

Open questions

- The behavior is unchanged for correctly reported file size, however
performances may vary because it uses the more complex sink interface.
Considering that sink is used a lot, I don't think this impacts the
performance a lot.
- `builtins.readFile` on an infinite file, such as `/dev/random` may
fill the memory.
- it does not support adding file to store, such as `${/proc/version}`.
The commit 3cc1125 adds a `grantpt`
call on the builder pseudo terminal fd. This call is actually only
required for MacOS, but it however requires a RW access to /dev/pts
which is only RO bindmounted in the Bazel Linux sandbox. So, Nix can
not be actually run in the Bazel Linux sandbox for unneeded reasons.
Only call grantpt on MacOS systems
Now it is always `drain` (see previous commit).
When used with `readFile`, we have a pretty good heuristic of the file
size, so `reserve` this in the `string`. This will save some allocation
/ copy when the string is growing.
Without dereferencing this pointer, you'd get an error like this:

```
error: unsupported argument 'abc' to 'fetchTarball', at 0x13627e8
```
Fix displaying error-position in `builtins.fetch{Tree,Tarball}`
This avoids inheriting the caller's shellHook, which can happen when
running a dev-shell inside a dev-shell.
edolstra and others added 24 commits June 11, 2020 15:45
Add an option to print the logs in a machine-readable format
This function was used in only one place, where it could easily be
replaced by readDerivation() since it's not
performance-critical. (This function appears to have been modelled
after queryDerivationOutputs(), which exists only to make the garbage
collector faster.)
This replaces the copy&paste with a helper function in hash.hh.
Use `std::string_view` in a few more places
This means that 'throw Error({ ... ErrorInfo ... })' now works.
These are now shown in the progress bar.

Closes NixOS#3577.
E.g. instead of

  error: --- BuildError ----------------------------------------------- nix
  builder for '/nix/store/03nk0a3n8h2948k4lqfgnnmym7knkcma-foo.drv' failed with exit code 1
  error: --- Error ---------------------------------------------------- nix
  build of '/nix/store/03nk0a3n8h2948k4lqfgnnmym7knkcma-foo.drv' failed

we now get

  error: --- Error ---------------------------------------------------- nix
  builder for '/nix/store/03nk0a3n8h2948k4lqfgnnmym7knkcma-foo.drv' failed with exit code 1
@mguentner
Copy link
Member

As the latest commit is really old, I would prefer to do a rebase.

How are your experiments going over at https://github.com/obsidiansystems/nix ?

@mguentner
Copy link
Member

For what it's worth, in the end I used a nginx reverse proxy to make the files available from ipfs. This does not require a patched nix daemon:

{
         "tarballs.example.com" = {
            forceSSL = true;
            enableACME = true;
            root = "/srv/ipfs/tarballs";
            locations."/" = {
              extraConfig = ''
                autoindex on;
              '';
            };
          };
          "cache.example.com" = {
            forceSSL = true;
            enableACME = true;
            root = "/srv/ipfs/binary_cache";
          };
          "channels.example.com" = {
            forceSSL = true;
            enableACME = true;
            root = "/srv/ipfs/channels";
            locations."/" = {
              extraConfig = ''
                autoindex on;
              '';
            };
          };
}

use together with https://github.com/NixIPFS/infrastructure/blob/master/ipfs_mirror/deploy.nix

@Ericson2314
Copy link
Collaborator Author

Ericson2314 commented Jun 15, 2020

Sure we can turn this into a rebase if you like, though these merges commit is already included in our history FWIW,

Our experiments are going good! It's true that the version here doesn't offer much over just using the HTTP gateway to some IPFS node, but we are about to leverage IPLD to represent the references graph, narfiles, etc., natively, which should up the value proposition for directly integrating in Nix by making pinning and other things much nicer.

@mguentner
Copy link
Member

Sure we can turn this into a rebase if you like, though these merges commit is already included in our history FWIW

@Ericson2314 I just gave you access to this repository. Feel free to push your branch(es). I pushed to https://github.com/NixIPFS/nix/tree/ipfs_binary_cache_legacy to preserve the single commit at the top for future reference.

@Ericson2314
Copy link
Collaborator Author

For the short term it's not just me but a bunch of us on the team that need access, so we'll continue using the company fork. But after we complete our initial proposal and our work transitions to a proper community project, I'd love to use this repo/organization for anything that isn't merged upstream, so thanks in advance.

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.