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 linking files into place anywhere on the system #1205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Samasaur1
Copy link
Contributor

This PR adds the system.file option (name open to bikeshedding), which allows users to place arbitrary files in arbitrary locations throughout their system. Usage ranges from simple cases such as:

system.file."Users/sam/Desktop/hello.txt".text = ''
  Hello, world!
'';

to more complex cases like this:

system.file = builtins.listToAttrs
  (builtins.map (jdk: 
    let
      versionElems = lib.strings.splitString "." jdk.version;
      majorVersion = builtins.elemAt versionElems 0;
    in
      lib.nameValuePair
      "Library/Java/JavaVirtualMachines/zulu-${majorVersion}.jdk"
      { source = "${jdk}/zulu-${majorVersion}.jdk"; }
  ) (builtins.attrValues { inherit (pkgs) zulu8 zulu11 zulu17 zulu; }));

Both of these examples will create symlinks, which I've tested and feel confident about.


The PR also aims to support copying files (identifying changes via SHA hashes), but I haven't landed on a great UI for doing so yet. Usage would look something like

system.file."Library/LaunchDaemons/org.nixos.activate-system.plist" = {
  source = thePlist;
  type = "copy";
};

The approach that seems to make the most sense to me is to also require a hash field in the above, but this is a pain to do from a user standpoint, so I'm open to suggestions.


I was also thinking that the PR would support a knownSHA256Hashes field, which would allow overwriting specific known files. I haven't gotten around to testing this yet, though.


As it stands, this PR doesn't change any existing behavior. Eventually, we will probably want to move all of the bespoke file-copying strategies to use this new system. I suspect that we will probably not want to swap the backend of environment.etc over, because I have made the deliberate decision not to have a "static directory" like /etc/static that makes activation more atomic. I'd be open to changing this, though.

@Samasaur1
Copy link
Contributor Author

I've been thinking more about how to copy files. Currently, I'm able to generate the JSON file entirely from Nix. If we want to keep this behavior, I don't think it's a good idea to add a hash field, in part because it's a pain for users who are copying files into place, but mostly because I don't see a way to do it for files generated by nix-darwin (such as LaunchAgents) without resorting to IFD.

What we could do is generate a JSON file from Nix, and then in the system.build command parse that JSON, hash each file that needs to be copied, and add those hashes to the JSON file before linking it into the newly built system. That way the hashes don't need to be specified, but are still computed and available to the linker script so that the hashes of files can be checked when switching to a new generation. I don't totally love this, but it's the best option I've come up with so far

@Enzime
Copy link
Collaborator

Enzime commented Dec 4, 2024

We could do something similar to the behaviour of home-manager, where if there's an existing file then it errors out unless you have home-manager.backupFileExtension set which only errors if a backup already exists and you need to create another backup

Comment on lines +33 to +39
target = mkOption {
type = types.str;
default = "/${name}";
description = ''
Name of symlink. Defaults to the attribute name preceded by a slash (the root directory).
'';
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note here that I've set the default target value to be the name of the attribute preceded by a slash. This means that when you configure a file, it looks like this:

system.file."opt/local/bin/hello".source = lib.getExe pkgs.hello

and a link will be created at /opt/local/bin/hello. I modeled this after environment.etc, where I think of the attr name as the path under /etc. I'm open to changing it if it seems unexpected, though

@Samasaur1
Copy link
Contributor Author

We could do something similar to the behaviour of home-manager, where if there's an existing file then it errors out unless you have home-manager.backupFileExtension set which only errors if a backup already exists and you need to create another backup

Well that's an orthogonal problem. Currently this PR errors on existing files unless they a) are a symlink identical to the one in the JSON file of the "current system"; b) are a non-symlink file whose hash matches knownSHA256Hashes; c) are a non-symlink file and the hash of the file is the same as the hash of that file in the JSON file of the "current system".

We need the hash in there so that new system generations that don't changed a copied file don't error on it. The problem I'm facing is how to get the hash in there in the first place

@Enzime
Copy link
Collaborator

Enzime commented Dec 4, 2024

home-manager doesn't track the content via hashes, they just consider any differences as a conflict and try to avoid overwriting by moving the old file out of the way, maybe we could do that instead of requiring hashes

@Samasaur1
Copy link
Contributor Author

Right, but they also don't support copying files. The problem is that if we want to support copying files (which I'd like to, so that we can replace the LaunchAgent stuff) and don't track by hash, then it's unreasonable to just error out at the presence of a file in the desired location, because you'd run into that problem every time you switch to a new generation, even if you don't change the file.

I guess what we could do is hash at activation time and if the hash of the existing file and the hash of the new file match, then continue, otherwise fail. (If this is what you were suggesting the whole time, my bad)

@Samasaur1
Copy link
Contributor Author

The downside of that last option is that it doubles the amount of time spent hashing during activation

@Samasaur1
Copy link
Contributor Author

Samasaur1 commented Dec 6, 2024

Okay, I've squashed and split and force-pushed, so this PR now only supports linking files. There's no support for copying files (so no support for a hash attribute), no support for knownSHA256Hashes (so any files in place will cause an error), and no static directory.

Support for copying files will come in a follow-up PR. Support for knownSHA256Hashes can either be added to this PR, added in the copying PR, or added in its own PR (I have the work already done) — up to y'all. The only other question I have is whether the system.file option name is good, or whether we should pick something else.

@Samasaur1 Samasaur1 changed the title Allow linking/copying files into place anywhere on the system Allow linking files into place anywhere on the system Dec 6, 2024
Comment on lines 135 to 136
# TODO: ensure enclosing directory exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one outstanding change for this PR is ensuring that the enclosing directories for new links exist. Copying something from Matrix:

  • Creating enclosing directories when files are linked into place originally: I haven't done this yet, because I wasn't sure if we just want to ensure that enclosing directories exist or also do something special to keep track of the fact that we made them so that we can remove them when necessary. If we do want to track this, how so? If we don't, see the next point:
  • Removing directories when files in them are removed: do we do this at all? how do we know when to do so? My current thought is that we only remove the actual files themselves, but if we remove the last file in a directory we also print a message during activation. I think this is reasonable because you'll only get the message as you switch from a generation that does have the nested file to a generation that doesn't have the nested file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and a follow-up from Matrix:

I don't really want to keep track of directories that we have to make, because then we're storing something that depends on the initial state of the user's filesystem, which means that it can't be computed purely, which in turn means storing it outside of /run/current-system, which I'm trying to avoid.

However, if we don't track these directories when we create them, we have two options:

  • leave intermediate directories (that were created by us) when we remove files from within them
  • remove any directories that become empty as a result of removing files within them (presumably recursively, though we could have a depth limit)

We could also have some option that lets users choose between these two behaviors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also a third option, which is to leave intermediate directories but print a warning whenever the actions taken from activation result in an empty directory. I think this is the best option

@Samasaur1
Copy link
Contributor Author

Tests are failing but it appears to be because of yabai. I do need to add tests, though

@Samasaur1 Samasaur1 force-pushed the system-files branch 5 times, most recently from 11714c3 to bef527c Compare December 15, 2024 20:57
@Samasaur1
Copy link
Contributor Author

Subject to the currently-running tests passing and any review comments, I think this is done. I decided not to track created intermediate directories and instead decided to print a message if we remove all the items from a directory and leave it empty. I also added a test, although since most of the logic of this PR is at activation time it doesn't cover all that much.

continue
if any(directory.iterdir()):
continue
print(f"The directory {directory} has been emptied; you may want to remove it")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should be printed in color or otherwise made more attention-grabbing?

@Samasaur1
Copy link
Contributor Author

Okay apparently the tests fail on stable because Python 3 is still Python 3.11, and I implemented tests by subclassing Path, which is only supported since 3.12

@Samasaur1
Copy link
Contributor Author

Hopefully that fixes the Python 3.11 problem

@Samasaur1
Copy link
Contributor Author

tests are still running but I think they're past the point where we saw the error last time

@Samasaur1
Copy link
Contributor Author

yeah, that one remaining failure is a Yabai error and not due to this PR

@Samasaur1
Copy link
Contributor Author

Samasaur1 commented Dec 30, 2024

I need to investigate (or better yet, write a test) for what happens when you do this:

system.file = {
  "tmp/hello.txt".text = ''
    Hello, world!
  '';
  "other.txt" = {
    target = "/tmp/hello.txt";
    text = ''
      Goodbye, world!
    '';
  };
};

I'd like it to gracefully error with a message about conflicts, but I don't know what lib.mapAttrs' does when you produce multiple elements with the same value. This may be solvable by calling something like lib.allUnique (lib.mapAttrsFlatten (name: value: value.target) rawFiles)

@Samasaur1
Copy link
Contributor Author

Added an assertion that there are no duplicate targets

@Samasaur1
Copy link
Contributor Author

Rebased on master and removed Python workaround

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.

2 participants