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

Move flake scripts out of overlays; fix format script #66

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

sebaszv
Copy link
Contributor

@sebaszv sebaszv commented Nov 11, 2024

Move flake scripts out from overlays

The flake scripts themselves are fine and do presently work with the current merge through the use of overlays.

As I've learnt, this can be problematic however.
View the following for reference of what is explained below:
https://discourse.nixos.org/t/why-does-overlaying-check-with-nodepackages-prettier-cause-a-massive-rebuild/55810

When overlaying packages, if there is an existing package with the same name, this should either be avoided altogether or done with consideration to ensure it is a proper replacement. check for example, is an existing package in nixpkgs.
Here a check overlay is defined, substituting it with the check script derivation defined in flake.nix. With he current batch of packages in the environment, nothing is affected fortunately. However, adding nodePackages.prettier will cause a massive rebuild by Nix as there are many packages that are dependent on the original check package. Nix, doing the right thing, works to build what it needs. Although the final result will be what is expected, this is more a side effect.

What is the point being argued here if the current implementation is working?

The big point is that this is a bad practice, since this is not what overlays were meant for. The same can be achieved by defining the packages anywhere else, and then referencing them as needed, rather than through overlays, which can cause unintended side effects, such as the one described.

Considering this is a repo that is very helpful particularly for beginners that may be new or not as experienced with the Nix ecosystem, presenting potential beginners to incorrect usage of overlays, can lead them to doing the same. Hence, the change here would be better in line with correct implementation. Someone visiting the repo hoping to implement helper script packages like here, could then reference this more "correct" implementation. Such as myself, who ended up following the previous implementation until I encountered the side effect explained in the post linked above.

The script derivations can really be defined anywhere, such as in a simple let binding, which I've done here. They will have gcroots linked so they won't be garbage collected, just like in the previous implementation.

In the end, the resulting behavior remains entirely the same, just the implementation is corrected to avoid side effects and serveas an example of better practices.

This is to claim any sort of superiority or anything regarding the previous implementation, rather I was hoping to improve this to save others the headache of going down the rabbit hole that I myself did, as expressed in the linked post.

Many thanks.

Fix format script

The previous script was failing, as it was supposed to. It was trying to match a file named '**/*.nix', rather
than expanding the intended pattern.

Bash doesn't do shell expansion inside of quotes. It must be written unquoted, else * and other special characters are treated literally.

'**/*.nix' -> **/*.nix

The new version will expand correctly.

Additionally, I've enabled globstar to make the expansion match all files ending with .nix recursively, including the top level. Without this, the pattern will only match up one level.

globstar is documented here:
https://www.gnu.org/software/bash/manual/html_node/The-Shopt-Builtin.html

@sebaszv sebaszv force-pushed the flake-scripts-overlays-remove branch from 82d6ce9 to f9ded69 Compare November 11, 2024 05:08
@sebaszv sebaszv force-pushed the flake-scripts-overlays-remove branch from f9ded69 to 82d6ce9 Compare November 18, 2024 21:36
The flake scripts themselves are fine and do presently
work with the current merge through the use of overlays.

As I've learnt, this can be problematic however.
View the following for reference of what is explained below:
<https://discourse.nixos.org/t/why-does-overlaying-check-with-nodepackages-prettier-cause-a-massive-rebuild/55810>

When overlaying packages, if there is an existing package with the
same name, this should either be avoided altogether or done
with consideration to ensure it is a proper replacement.
`check` for example, is an existing package in `nixpkgs`.
Here a `check` overlay is defined, substituting it
with the `check` script derivation defined in `flake.nix`. With
the current batch of packages in the environment, nothing is affected
fortunately. However, adding `nodePackages.prettier` will cause
a massive rebuild by Nix as there are many packages that are dependent
on the original `check` package. Nix, doing the right thing, works to
build what it needs. Although the final result will be what is expected,
this is more a side effect.

What is the point being argued here if the current implementation is
working?

The big point is that this is a bad practice, since this is
not what overlays were meant for. The same can be achieved by defining
the packages anywhere else, and then referencing them as needed,
rather than through overlays, which can cause unintended side effects,
such as the one described.

Considering this is a repo that is very helpful particularly for
beginners that may new or not as experienced with the Nix ecosystem,
presenting potential beginners to incorrect usage of overlays, can
lead them to doing the same. Hence, the change here would be better
in line with correct implementation. Someone visiting the repo
hoping to implement helper script packages like here, could then
reference this more "correct" implementation. Such as myself,
who ended up following the previous implementation until I encountered
the side effect explained in the post linked above.

The script derivations can really be defined anywhere, such as in a
simple `let` binding, which I've done here. They will have gcroots
linked so they won't be garbage collected, just like in the previous
implementation.

In the end, the resulting behavior remains entirely the same, just
the implementation is corrected to avoid side effects and serve
as an example of better practices.
The previous script was failing, as it was supposed to.
It was trying to match a file named `'**/*.nix'`, rather
than expanding the intended pattern.

Bash doesn't do shell expansion inside of quotes.
It must be written unquoted, else `*` and other
special characters are treated literally.

`'**/*.nix'` -> `**/*.nix`

The new version will expand correctly.

Additionally, I've enabled `globstar` to make the
expansion match all files ending with `.nix`
recursively, including the top level. Without this,
the pattern will only match up one level.

`globstar` is documented here:
<https://www.gnu.org/software/bash/manual/html_node/The-Shopt-Builtin.html>
@sebaszv sebaszv force-pushed the flake-scripts-overlays-remove branch from 82d6ce9 to 66939f5 Compare November 18, 2024 21:58
@lucperkins
Copy link
Contributor

Yeah, seems reasonable

@lucperkins lucperkins merged commit 7f906fa into the-nix-way:main Nov 19, 2024
6 checks passed
@sebaszv sebaszv deleted the flake-scripts-overlays-remove branch November 19, 2024 02:35
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