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

feat(home-manager): cava,imv,mako,swaylock sans IFD #377

Closed
wants to merge 1 commit into from

Conversation

mightyiam
Copy link

Closes #340
Closes #341
Closes #375
Closes #376
Closes #373

@mightyiam mightyiam marked this pull request as ready for review November 14, 2024 03:34
Copy link
Member

@isabelroses isabelroses left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

My only real concern is maintainability. Seeing how its a bit more complex then anything we have currently and there is no spec, but rough commonalities. This will be especially apparent when we have more ports that are using INI.

So I'm willing to approve but would like to ask @getchoo to confer.

@mightyiam
Copy link
Author

May I help with whatever is preventing this from merging?

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

Sorry for the wait, this ended up a bit too far back in my inbox. Thanks for bumping it!

The implementation of fromINI here is really good and I would accept it. However, this isn't actually preventing IFD...or it shouldn't anyways

Currently we use npins to manage our sources. npins fetches these sources through the builtin fetchers, which unlike those found in Nixpkgs, block evaluation by downloading the paths -- just like a normal instance of IFD would. This isn't considered IFD by Nix itself though, as the builtin fetchers aren't technically outputting derivations.

To put it simply: our use of npins and builtin fetchers for our sources allows this

(builtins.readFile (sources.imv + "/themes/${cfg.flavor}.config")

to not be considered IFD, even though builtins.readFile <expr> is a textbook example of IFD. The solution here only takes advantage of this to avoid the requirement of the --allow-import-from-derivation flag, while still having the same blocking effects on evaluation; it will also no longer work once we move to Nixpkgs fetchers. This makes it very difficult to justify bringing this function in tree, as the only difference soon will be in this being a pure Nix solution rather than calling an external command -- which while cool in theory, doesn't bring much practical benefit here

I strongly encourage sharing this implementation though. For local files it would work wonders, and I'm sure prevent IFD in many other projects. nixpkgs.lib may be a good place :)

@getchoo getchoo closed this Nov 23, 2024
@mightyiam
Copy link
Author

The solution here only takes advantage of this to avoid the requirement of the --allow-import-from-derivation flag

Sorry, I lost you there. I'm not sure what the meaning of "here" is, nor what "this" is.

@getchoo
Copy link
Member

getchoo commented Nov 25, 2024

"here" is referring to this pull request. "this" is referring to the way I described builtin fetchers blocking evaluation similar to IFD, without technically causing IFD

@mightyiam
Copy link
Author

So I am to understand that a change is being considered and possibly worked on, that will replace the use of builtin fetchers with nixpkgs fetchers. And that a result of that change is that IFD will be unavoidable for all modules?

@getchoo
Copy link
Member

getchoo commented Nov 25, 2024

So I am to understand that a change is being considered and possibly worked on, that will replace the use of builtin fetchers with nixpkgs fetchers

Yes

And that a result of that change is that IFD will be unavoidable for all modules?

No

Most modules that currently do not require IFD will continue to not -- in fact they will actually see a major improvement in the downloads of their sources no longer blocking evaluation. Some modules that take advantage of the use of builtin fetchers to technically not perform IFD will now require it, though. The changes in this PR are an example of the latter case.

@mightyiam
Copy link
Author

Okay, I understand enough that I would like to ask some more about it, but is there a better thread for that, please?

@getchoo
Copy link
Member

getchoo commented Nov 25, 2024

The introductory paragraph of the "Fetchers" chapter in the Nixpkgs manual is a good resource. We've also recently opened discussions for this repository, so feel free to open a thread there :)

@mightyiam
Copy link
Author

Thank you. May I see this WIP upcoming change, please?

@getchoo
Copy link
Member

getchoo commented Nov 27, 2024

#384

This was referenced Nov 30, 2024
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.

cava sans IFD imv sans IFD fromINI sans IFD mako sans IFD swaylock sans IFD
3 participants