mkDevos and evalDevosArgs: Move flake implementation logic into lib#206
mkDevos and evalDevosArgs: Move flake implementation logic into lib#206Pacman99 wants to merge 18 commits intodivnix:corefrom
Conversation
|
Very interesting. I had another exhausting day, so I won't even try to review this right now. But I'll definitely have a good look at this soon. |
blaggacao
left a comment
There was a problem hiding this comment.
This totally satisfies my desire for clear interfaces.
I've taken a first read and I find it really easy to reason about it.
It is really comfortable and totally magic-less to have mkDevos.nix give an overview of the default file system layout in only aboy 10 lines from the function arguments.
I have a few associations, but I'll keep them for opening new issues and discuss them individially / incrementally once this general direction is merged.
|
Hey, I am the guy from Reddit - I have to say, this really does look like my project :D Good stuff tho, hopefully, this will get merged! :) |
|
@Pacman99 totally open ended question: what benefits and drawbacks would you see in making this a |
Actually now that you mention that, I was actually considering setting up a fork of devos that exported all its features as a lib function. I would really like to not do that, but if I did I would keep it as close to devos as possible and just add a lib function layer. But it would be much better if it was in devos instead. I mainly wasn't sure if you all wanted devos to go in this direction of being a lib function rather than a template. This PR attempts to make a compromise by doing both. Move everything to lib but maintain folder structure/template. Also there is one feature I like from |
Very wise choice. I agree, before v1.0, or even beyond that point, it is a feature to be able to hack one's own |
|
it's all you @Pacman99 |
5dba721 to
7122fa1
Compare
| lib.optionalAttrs | ||
| (lib.pathExists "${dir}/${n}/default.nix") | ||
| { default = /. + "${dir}/${n}"; } | ||
| { default = builtins.toPath "${dir}/${n}"; } |
There was a problem hiding this comment.
This is a deprecated function, but without this I get an error that store paths in strings can't be appended to paths.
I tried builtins.path too and that didn't work. I think this could be reported upstream, I'm just not sure what to report.
There was a problem hiding this comment.
Not sure what you mean. It is already a string. But I did try toString /. + .... That didn't work, but it is redundant since it defeats the point of the original change.
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild succeeded: |
|
Update: I added a |
|
Dropped the |
|
Update: created a mkdevos template with a list of excluded folders that aren't needed when using mkdevos. And some general doc cleanups. |
|
|
…ent flake check error
Reason: the authors probably believed, as I do, that there are people who notice the fine matices of overzealous "branding" and percieve it as obnauxious. Code becomes more concise, if one is able to read it out in plain english, hence:
I'd vote for "devos, make a flake if yours" which has most informational value per char and is in line with stances of Note that, with flakes as opposed to traditional global |
There was a problem hiding this comment.
Ha, my parts of my review from two days ago survied battery die-out in the cache...
I'll request some changes in naming and docs to polish this off.
The docs might need a few iterations, still. I'm concerned with just throwing "random" options without super clear use cases or context at users in plain nix-ecosystem-style. To reach our goals, this is one of the in my opinion bad practices in the nix ecosystem, I don't think we should inherit.
The docs should answer more questions, than they generate. Here, I'm especially concerned to have a cryptic "Use mkDevos" in the index in absence of a clear self-identifying use-case in the title, like "How to get started" or "Use devos as a library!" — with exclamation mark it is a recommendation (with impetus) and I think in line what we actually want to recommend (with impetus) in the future.
Then we could elaborate around that central theme "use it as a library" as a three paragrapher with a structure around the lines of either of e.g.
- problem — solution — benefit
- what? — now what? — so what?
- benefit — solution — example
I could offer to have a take on the docs, if you want, but it would be some days into the future still, until I have again full dominion over my working workstation. Let me know.
lib/devos/default.nix
Outdated
| config = { allowUnfree = true; }; | ||
| }; | ||
|
|
||
| importIf = path: if builtins.isPath path || builtins.isString path |
There was a problem hiding this comment.
flake-utils/simple flake calls this maybeImport. Since the condition is implicit we neither know nor care at the calling site, so we might copy that naming.
There was a problem hiding this comment.
Just realized, its different here. But as elaborated in conjunction with my other comments, it probably should not be.
Coming from that tacit stance which slowly unrevealed itself to me in proper shape, is probably why I got confused in the first place.
| ``` | ||
| then replace the outputs section with this: | ||
| ```nix | ||
| outputs = { self, devos, ... }: devos.lib.mkDevos { inherit self; }; |
There was a problem hiding this comment.
From a clean naming point of view this should be devos.lib.mkFlake.
"Make a flake with devos". Else we immediatly have a reasoning issue:
What is this Devos, that you can make with devos?!?
There was a problem hiding this comment.
devos.mkFlake for external devos-as-a-library users.
| @@ -0,0 +1,36 @@ | |||
| # Use mkDevos | |||
| You can also use devos as a flake input and get the advantage of using | |||
There was a problem hiding this comment.
I'm not a big fan of just throwing options at the user. It increases complexity and defeats the purpose of being easy-onboarding.
We should clearly blueprint the use case in simple english so that users can identify themselves as "yes that's me, I'll do it this way".
devos.lib
-
If you think devos template is tpp complicated ...
-
Once we are 1.0, don't fork this (!), use it as a lib instead...
-
...
There was a problem hiding this comment.
I think for now it should be documented as a 'next step' and another option. My reasoning is that there is some work to switch to the mkDevos method, and it's not an easy change.
| nix flake update --update-input devos | ||
| ``` | ||
|
|
||
| If you would like to change your directory's structure, you can pass |
There was a problem hiding this comment.
I'm not a big fan of nobleing this into the docs as a stand-alone use case: I'm not sure if preference is a use case motivated all by itself.
Also we should not throw options at users without clear guidance in what situations they have to use which option.
| look at [evalDevosArgs](../../lib/devos/evalDevosArgs.nix) to learn about | ||
| the arguments that it can take. | ||
|
|
||
| > ##### Notes: |
There was a problem hiding this comment.
A few too many notes, I guess. If it merits exhibition, then it probably also deserves more than a note.
We should find other means for them.
There was a problem hiding this comment.
My thoughts too, the docs need some work
| > the [core profile](../../profiles/core) | ||
| > - You can append `/community` to the `devos.url` to get access to community modules | ||
| > which can be added in [extern](../../extern). | ||
| > - To use community profiles, you will have to copy them to your tree |
There was a problem hiding this comment.
Why? — asks the reader (not me).
Is it relevant (enough) to mention? Can't it be solved with better UX?
There was a problem hiding this comment.
Well its because profiles are not exported anywhere in the flake. I don't see the point in adding the why to docs. And also at some point we could try to export profiles under nixosModules to fix this.
| > - You can append `/community` to the `devos.url` to get access to community modules | ||
| > which can be added in [extern](../../extern). | ||
| > - To use community profiles, you will have to copy them to your tree | ||
| > - If you use this method, you will no longer be able to edit devos internals. |
There was a problem hiding this comment.
I think this can be dropped. Rather we need to define clear guidance when users are expected to use which method:
- library
- template
- fork
I think maybe we should avoid (end user) documenting this PR alltogether at this moment and rather have a discussion about what usage modes we recommend for which situation.
There was a problem hiding this comment.
I would appreciate being told this, just to make it clear the cons of using mkDevos.
| @@ -0,0 +1,103 @@ | |||
| { self, dev, nixos, inputs, ... }: | |||
There was a problem hiding this comment.
dev. This PR is totally the wrong place to complain, but every time I read it, I have to go through an uncounted number of mental hops to figure out what it means. 😕
What is it? (Can we make that evident from its name?)
There was a problem hiding this comment.
I don't like dev either, it is kind of confusing. I'm not sure how to fix it though outside of better documentation. Its just a side effect of needing to export lib as lib in the flake, but also requiring a way to access devos lib within other devos lib functions.
There was a problem hiding this comment.
perhaps simply renaming to osLib or devLib. I really just went with dev to keep it as convenient as lib, i.e. three chars.
There was a problem hiding this comment.
I personally think dev is ok, my main issue is the fact that lib in some places refers to devos lib and in other places refers to nixos lib. perhaps we can just standardize dev and use it everywhere even in flake.nix. So dev always refers to devos lib and lib always refers to nixos lib. I'm confusing myself nevermind. We should definitely export the devos lib as lib in flake.nix.
| argOpts = with nixos.lib; { config, ... }: | ||
| let | ||
| inherit (dev) os; | ||
| inherit (os) importIf; |
There was a problem hiding this comment.
Could that be devos.lib.maybeImport just as if any external lib consumer would imvoke it at the calling site? The os token is similarily unfortunate as is dev in my opinion.
Or to align with how we import:
import (devos.lib) maybeImport
There was a problem hiding this comment.
Looka like it would be maybeImportElse, now.
There was a problem hiding this comment.
I'm also not sold on not declaring it locally as long as it is not of actual use in another place.
There was a problem hiding this comment.
I could just declare it in evalDevosArgs, its really only useful there.
Still thinking of names, importWithFallback is along the lines of what I'll probably go with.
There was a problem hiding this comment.
Should not be necessary with mkFlake
lib/devos/evalDevosArgs.nix
Outdated
| default = "${self}/users"; | ||
| description = "path to folder containing user profiles"; | ||
| }; | ||
| extern = mkOption { |
There was a problem hiding this comment.
Instead kf encapsulating this within extern, I would prefer those things to be declared at the top level flake.nix.
Those are even higher level than suites.nix, which I'm going to propose below.
We should just make them direct attributes of mkDevos / mkFlake, that is top level options in here.
There was a problem hiding this comment.
Yeah, this is part of the prime devos api, we should refrain from putting it into a badly named extern config object.
Why 2 levels of api?!?
There was a problem hiding this comment.
I see your point, but I would also prefer to address things like this in a separate discussion/PR. For now I want mkDevos/mkFlake to just be a mirror of the current devos api. So that way we can separate discussion and lib implementation.
blaggacao
left a comment
There was a problem hiding this comment.
Follow up with went was missing above...
| name = lib.removeSuffix ".nix" (baseNameOf path); | ||
| name = lib.removeSuffix ".nix" | ||
| # Safe as long this is just used as a name | ||
| (builtins.unsafeDiscardStringContext (baseNameOf path)); |
There was a problem hiding this comment.
What's motivating this change?
(And typo in the comment)
There was a problem hiding this comment.
see pr that changes pathsToImportedAttrs I put reasoning there.
| argOpts = with nixos.lib; { config, ... }: | ||
| let | ||
| inherit (dev) os; | ||
| inherit (os) importIf; |
There was a problem hiding this comment.
Looka like it would be maybeImportElse, now.
| config = { allowUnfree = true; }; | ||
| }; | ||
|
|
||
| importIf = path: fallback: |
There was a problem hiding this comment.
This does not allow to declare things inline (within the flake). There is no reason to enforce library users to use any sort of file hierarchy.
To reflect that, rather let's use this implementation:
https://github.com/numtide/flake-utils/blob/master/simpleFlake.nix#L31
There was a problem hiding this comment.
Big reverbaration step of mine, see other comment about my thought process, here.
There was a problem hiding this comment.
Yeah with the planned mkFlake I don't think the 'importIf' function will be necessary. I really only made it to maintain backwards compatibility so mkDevos could be used on devos itself. But I see now why that would be better done outside of lib.
| description = "The flake to create the devos outputs for"; | ||
| }; | ||
| hosts = mkOption { | ||
| type = path; |
There was a problem hiding this comment.
I'm not convinced of any benefits of encoding any specific file hierarchy into the library's API. The library should require to abstractly fulfill its own requirements and nothing more.
How people organize their files does not matter as long as their contents fulfill the devos requirement.
This stance complements my call for maybeImport: people can also declare functions directly within the top level flake.
Note, that flake.nix will shrink dramatically. Hence the only sane purpose we can give it is "index". We should not hide that away into a library function.
There was a problem hiding this comment.
An alternative is to layer another funktion:
devos.mkDevosFlake (to encode file hierarchy) over devos.mkFlake, but we really forego a stripped down flake.nix "index"-service if we don't always specify paths directly within the flake.nix.
So we shoudn't do a mkdDevosFlake / mkDevos at all: the "devos" part in mkDevos would actually become a template. That is much more inclusive of potential future library users.
There was a problem hiding this comment.
Will address in planned mkFake PR. So only devos encodes its own filesystem api within its own flake.nix, but its not encoded within lib.
| @@ -0,0 +1,24 @@ | |||
| { lib, dev, ... }: | |||
There was a problem hiding this comment.
As promised above, I wanted to suggest to declare suites within flake.nix or at least within top level suites.nix.
Suites are a higher level aggregate of profiles and users, their definition is ultrashort. I don't see a point — by reason of analogy — to burry them in a folder (at the same level as profiles).
There was a problem hiding this comment.
Well this function doesn't actually declare suites, it just processes them. suites are still declared in top level and can be declared within the flake. The goal here was to prevent suites from having to do the work of importing profiles and processing them.
|
I'm sorry for my convoluted feedback. If it results impossible / inefficient to consume, please split up this PR into its atomic parts, and I'll re-apply a more focused feedback on each increment. After reverberation in my thoughts, I came to the conclusion, we ought to make any devos library agnostic of any file hierarchy layout. For indexing purpose, file hierachy needs to be declared in (a henceforth stripped down) top level File layout is best encoded as "batteries included but removable" within a template. An abstract devos's library is also attractive to more people (wink at @gytis-ivaskevicius 's impetus) and thereby promotes "consensus".
A rough api design for library users would look like this: (No point in putting We then could reflect that in devos-the-template and adopt it for in-tree devos-the-blueprint (aka core/community): |
|
Note: With devos-a-library there is still a need for This is a proper fix in preparation for #166 |
gytis-ivaskevicius
left a comment
There was a problem hiding this comment.
Can't wait to see this merged :)
| inputs = { | ||
| ... | ||
| devos.url = "github:divnix/devos"; | ||
| }; | ||
| ``` | ||
| then replace the outputs section with this: | ||
| ```nix | ||
| outputs = { self, devos, ... }: devos.lib.mkDevos { inherit self; }; |
There was a problem hiding this comment.
Consider instead of specifying individual changes be like "this is how your flake should look like" and users can add extra attributes if necessary
There was a problem hiding this comment.
I like that idea. I think docs should be its own PR too, discussion for that can happen on its own.
| deploy.nodes = os.mkNodes deploy self.nixosConfigurations; | ||
| outputs = inputs@{ deploy, nixos, nur, self, utils, ... }: | ||
| let | ||
| lib = import ./lib { inherit self nixos inputs; }; |
There was a problem hiding this comment.
Just a little idea for the future - separate the library implementation from this template repository
(probably not relevant as part of this PR)
There was a problem hiding this comment.
I think I would be for that. @nrdxp @blaggacao thoughts?
There was a problem hiding this comment.
Definitly, in the coming days, until 1.0 we might need the benefit of atomic spanning PRs, still, though.
| @@ -21,7 +21,9 @@ rec { | |||
| paths' = lib.filter (lib.hasSuffix ".nix") paths; | |||
| in | |||
| genAttrs' paths' (path: { | |||
There was a problem hiding this comment.
In Nix usually, collection itself (in this case paths') is specified as the last argument. While we are at it - maybe it's time to change it as well?
| check = builtins.isFunction; | ||
| description = "Nixpkgs overlay"; | ||
| }; | ||
| default = importIf "${self}/pkgs" (final: prev: {}); |
There was a problem hiding this comment.
It makes me wonder if users won't get confused at first that something gets picked up by just creating a folder (without manually declaring it)
There was a problem hiding this comment.
Good point, I should be able to address this soon when switching to mkFlake which will be more general and then devos will use mkFlake on itself. That way the importIf is not necessary either.. @blaggacao I think this is a good example of the corruption you mentioned.
| }; | ||
|
|
||
| importIf = path: fallback: | ||
| if builtins.pathExists path then import path else fallback; |
There was a problem hiding this comment.
Won't it crash if path does not contain default.nix file?
There was a problem hiding this comment.
Yes, but this is supposed to be a thin wrapper around nix's import and that would crash if there is no default.nix.
| tests = nixos.lib.optionalAttrs (system == "x86_64-linux") | ||
| (import "${devos}/tests" { inherit self pkgs; }); |
There was a problem hiding this comment.
other platforms not supported????
There was a problem hiding this comment.
Unfortunately not yet, the tests in devos need a bit of reworking for that to happen. But other platforms will be checked using deploy-rs checks, so its not terrible.
| nix.registry = { | ||
| devos.flake = self; | ||
| nixos.flake = nixos; | ||
| override.flake = override; | ||
| override.flake = inputs.override; | ||
| }; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah I saw that earlier when looking through flake-utils-plus. I've actually had an issue with that though, and I've been meaning to bring it up in devos at some point. When you evaluate your flake after building the registry, the lock file will change to reference paths in your nix store instead of the url's to the inputs. And this sort of reduces reproducability since you can't evaluate your flake on other machines as easily. I ran across this while setting up CI for my server, it kept failing since nix was trying to reference store paths that didn't exist in the CI machine.
| nix.nixPath = [ | ||
| "nixpkgs=${nixos}" | ||
| "nixos-config=${self}/compat/nixos" | ||
| "home-manager=${home}" | ||
| "home-manager=${inputs.home}" | ||
| ]; |
There was a problem hiding this comment.
I am getting rid of NIX_PATH in an upcoming release of flake-utils-plus, it does not seem to be particularly relevant in the world of Nix 3.0
Feel free to keep it - tho I'd suggest checking this one out https://discourse.nixos.org/t/i-feel-like-nix-path-creates-more-issues-than-it-fixes/12110/9
There was a problem hiding this comment.
I have to agree, NIX_PATH is not the best. I don't know if we will remove this anytime soon though, one of devos goals is to help with the transition to flakes. This part should definitely be removed eventually.
| allProfiles = | ||
| let defaults = lib.collect (x: x ? default) profiles; | ||
| in map (x: x.default) defaults; | ||
|
|
||
| allUsers = | ||
| let defaults = lib.collect (x: x ? default) users; | ||
| in map (x: x.default) defaults; |
There was a problem hiding this comment.
Consider defining myFunc = it: lib.collect (x: x ? default) it;
There was a problem hiding this comment.
Yeah good idea, since I'm already changing the suites implementation logic in #216 I can also address this.
| @@ -1,9 +1,8 @@ | |||
| { self ? (import ../compat).defaultNix | |||
| , system ? builtins.currentSystem | |||
| , pkgs ? import <nixpkgs> {} | |||
There was a problem hiding this comment.
???? is this necessary? nix develop should take care of nixpkgs, not NIX_PATH
There was a problem hiding this comment.
Adding the pkgs part is necessary so that the shell implementation doesn't try to create its own instance of pkgs and so it can be called within lib.
The import <nixpkgs> {} part is there because the shell folder is also there for backwards compatibility. So you can still do nix-shell. Without it, nix-shell would error out.
There was a problem hiding this comment.
Maybe we can use the back-compat flake (anti corruption layer) which iirc handles also shell back-compat for this class of use cases.
There was a problem hiding this comment.
this has been addressed in #217 the solution has some potential for corruption, but only when using old nix-shell, so I don't think its all that bad.
|
Thanks @gytis-ivaskevicius and @blaggacao for all the review! As always the feedback is amazing. Following @blaggacao suggestion, I'm splitting up all these changes into separate PR's, which can each be merged by themselves. So I'll probably end up marking this one as a draft and make a new PR for just the change that involves moving flake logic into lib. |
|
closing in favor of #218 |
Moving all logic to lib makes things easier to change and understand, since whenever you add a feature, you only have to edit relevant functions in lib. And for a user, they don't have to understand whats happening behind the scenes. But if they want to I think reading functions in lib is simpler than reading implementation logic thats in different places around the directory.
But most importantly this allows for a lib function called
mkDevoswhich is a potential solution to #91. The mkDevos function can take other an input of other flakes and build a devos flake just like this repo does. So this creates an alternate method of using devos, where you just add devos as an input then doAnd then you can just create the relevant directories(this could be shipped as its own template) and those directories will be imported. This change is also backwards compatible as demonstrated in the updated flake.nix where the current devos just calls
mkDevoswith itself. So a user can either just clone this repo and use it like its currently being used, or they can use the lib function.. The advantage of just using the lib function is you no longer have to worry about merging upstream and conflicts that arise, you can just update your devos input, so nix flake takes care of syncing with the mothership for you.Keep in mind you have to add all inputs devos depends on along with devos for the new method to work because of an issue with the way
followsworks in flakes. This should be fixed by NixOS/nix#4641 which would allow you to write a devos flake.nix in under 10 lines.