nixos/lib/utils: Add fileSystems.<name>.depends option and generalise fsBefore (fixes #86955)
#86967
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakobrs
changed the title
This comment has been minimized.
This comment has been minimized.
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
481d8d9
to
aba704a
Compare
jakobrs
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jakobrs
force-pushed
the
more-general-fsbefore
branch
2 times, most recently
from
6cbb64e
to
8ccc574
Compare
ofborg
bot
added
8.has: module (update)
10.rebuild-linux: 1-10
10.rebuild-linux: 1
and removed
10.rebuild-linux: 0
labels
jakobrs
changed the title
fileSystems.<name>.depends option and generalise fsBefore (fixes #86955)
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
2.status: stale
dotlambda
reviewed
Feb 2, 2021
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
8ccc574
to
e350a5e
Compare
nrdxp
approved these changes
May 25, 2021
There was a problem hiding this comment.
I am successfully using this to work around an issue when combining impermanence and agenix
It's working beautifully so I believe it is ready for merging
dotlambda
reviewed
May 27, 2021
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
e350a5e
to
31c4b44
Compare
|
Well when I originally made this #95406 wasn't even created yet. I'm not entirely convinced the |
dotlambda
reviewed
May 27, 2021
Comment on lines
+27
to
+28
| nonEmptyWithoutTrailingSlash = addCheckDesc "non-empty without trailing slash" types.str | ||
| (s: isNonEmpty s && (builtins.match ".+/" s) == null); |
There was a problem hiding this comment.
Is there a reason for these not to start with a /?
Suggested change
| nonEmptyWithoutTrailingSlash = addCheckDesc "non-empty without trailing slash" types.str | |
| (s: isNonEmpty s && (builtins.match ".+/" s) == null); | |
| nonEmptyPathWithoutTrailingSlash = addCheckDesc "non-empty without trailing slash" types.path | |
| (s: isNonEmpty s && (builtins.match ".+/" s) == null); |
Note that this doesn't make much sense:
- any
pathis non-empty - the path
/has a trailing slash but is still allowed (even before this change)
There was a problem hiding this comment.
I just copied the check used by #95406
There was a problem hiding this comment.
I know you did. I guess we can leave improving this to a different PR.
dotlambda
reviewed
May 29, 2021
jakobrs
force-pushed
the
more-general-fsbefore
branch
2 times, most recently
from
bdf293e
to
76b6a3c
Compare
dotlambda
reviewed
May 29, 2021
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
76b6a3c
to
cd716e6
Compare
dotlambda
reviewed
May 29, 2021
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
cd716e6
to
e6cbf44
Compare
nrdxp
approved these changes
Jun 1, 2021
nrdxp
approved these changes
Jun 1, 2021
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
talyz
reviewed
Jun 8, 2021
nixos/lib/utils.nix
Outdated
Comment on lines
38
to
40
| in hasPrefix a'.mountPoint b'.device | ||
| || hasPrefix a'.mountPoint b'.mountPoint | ||
| || any (dependency: hasPrefix a'.mountPoint dependency) b'.depends; |
There was a problem hiding this comment.
Thanks for doing this! I just have a small stylistic suggestion, but other than that, I think this looks ready to be merged.
Suggested change
| in hasPrefix a'.mountPoint b'.device | |
| || hasPrefix a'.mountPoint b'.mountPoint | |
| || any (dependency: hasPrefix a'.mountPoint dependency) b'.depends; | |
| in | |
| hasPrefix a'.mountPoint b'.device | |
| || hasPrefix a'.mountPoint b'.mountPoint | |
| || any (hasPrefix a'.mountPoint) b'.depends; |
There was a problem hiding this comment.
@jakobrs It ended up in the wrong commit and is still missing the formatting. While the indentation means the hasPrefix lines don't line up as neatly, it shows more clearly on a quick glance what belongs to the expression, separating it from the let .. in.
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
e6cbf44
to
c4d00ed
Compare
jakobrs
force-pushed
the
more-general-fsbefore
branch
from
c4d00ed
to
ea34fe2
Compare
|
The commit which modifies I suspect the problem is that some of my mounts have a The root filesystem is mounted using a device, but the filesystem (BTRFS) has the label "NIXOS". This label is then used to mount additional subvolumes. |
|
@emmanuelrosa I guess we could just add let
getDevice fs =
if fs.device != null then
fs.device
else if fs.label != null then
"/dev/disk/by-label/${fs.label}"
else
""; # This will be caught when building the fstab, and we'd rather throw there
normalisePath = path: "${path}${optionalString (!(hasSuffix "/" path)) "/"}";
normalise = mount: mount // { device = normalisePath (getDevice mount);
mountPoint = normalisePath mount.mountPoint;
depends = map normalisePath mount.depends;
};This would have the advantage of having "more correct" semantics, although it's a little more complex. |
11 tasks
|
Here is a PR: #127309 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
See #86955 for an example configuration where this matters.
What's new
The PR adds a
fileSystems.<name>.dependsoption that can be used to specify what other mount points need to be mounted before this one, and makesfsBeforerecognise more forms of dependencies "out of the box", to fix #86955.Note: The following three comments of mine were created before a force push and no longer apply.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)