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

treewide: fix eval related to with lib; removal #339356

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

eclairevoyant
Copy link
Contributor

@eclairevoyant eclairevoyant commented Sep 3, 2024

Description of changes

fixup for #335603, #335618

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@eclairevoyant
Copy link
Contributor Author

eclairevoyant commented Sep 4, 2024

I can't promise that this fixes every problem but I fixed what I saw

Basically my approach was to use the following script:
#!/usr/bin/env bash

# Get the list of files containing "with lib;"
files=$(rg -lFx "with lib;" ./nixos/)

optionstring=$(nix eval --impure --expr 'with import <nixpkgs> {}; builtins.attrNames (builtins.removeAttrs lib ["__unfix__" "licenses" "fix" "and" "or" "kernel" "options" "meta" "path"])' | sed 's/" "/ /g;s/" ]//g;s/\[ "//g')
IFS=' ' read -ra options <<< $optionstring

# Function to escape special characters for sed
escape_sed_pattern() {
    echo "$1" | sed 's/[.[\*^$+()|]/\\&/g'
}

# Loop over each file
for file in $files; do
    # delete empty before
    sed -i '/with lib;/,$ b; /^$/d;' "$file"
    # delete empty after
    sed -i '/with lib;/{N;s/\n$//}' "$file"
    # delete match
    sed -i '/^\s*with lib;\s*$/d' "$file"

    for str in "${options[@]}"; do
        # Escape the string for use in sed
        escaped_str=$(escape_sed_pattern "$str")\\b

        # Prepend "lib" to the current string
        lib_str="lib.$str"

        # Escape the lib_str for use in sed
        escaped_lib_str=$(escape_sed_pattern "$lib_str")

        # Use sed to replace occurrences in the temporary file
        sed -i '/inherit/!s/\([^-._0-9a-zA-Z]\)'"$escaped_str"'/\1'"$escaped_lib_str"'/g' "$file"
    done
done

Key modifications were to use an exclusion list against lib rather than an inclusion list, and I added an additional check to ensure that the previous character wasn't a word-character and that the string ended on a word boundary.

Then I diffed the result against master, only looking at the files involved in the previous PRs, and manually reviewed + applied only the changes that seemed correct.

@philiptaron
Copy link
Contributor

philiptaron commented Sep 4, 2024

Almost all of these fixes are not actually breaking.

These symbols are in the Nix prelude, are available without any import, and also happen to be available from lib:

  1. map
  2. removeAttrs

These symbols are in the Nix prelude, are available without any import, and are not available in lib:

  1. abort
  2. baseNameOf
  3. break
  4. builtins
  5. derivation
  6. derivationStrict
  7. dirOf
  8. false
  9. fetchGit
  10. fetchMercurial
  11. fetchTarball
  12. fetchTree
  13. fromTOML
  14. import
  15. isNull
  16. null
  17. placeholder
  18. scopedImport
  19. throw
  20. toString
  21. true

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

There are four real issues here, three that affect eval/functionality and one that affects diagnostics. The rest of the files can be reverted or committed, it doesn't really matter.

I prefer to revert, since map is shorter than lib.map, but I'll leave that up to you, @eclairevoyant.

nixos/modules/config/i18n.nix Show resolved Hide resolved
nixos/modules/config/resolvconf.nix Show resolved Hide resolved
@eclairevoyant
Copy link
Contributor Author

These symbols are in the Nix prelude, are available without any import, and also happen to be available from lib:

Thanks for pointing it out, I've removed those changes.

@anpin
Copy link
Contributor

anpin commented Sep 4, 2024

please add this as well

diff --git a/nixos/modules/services/hardware/thinkfan.nix b/nixos/modules/services/hardware/thinkfan.nix
index 9dd4c5434211..9733fbe5aa15 100644
--- a/nixos/modules/services/hardware/thinkfan.nix
+++ b/nixos/modules/services/hardware/thinkfan.nix
@@ -12,7 +12,7 @@ let
       tuple = ts: lib.mkOptionType {
         name = "tuple";
         merge = lib.mergeOneOption;
-        check = xs: all id (zipListsWith (t: x: t.check x) ts xs);
+        check = xs: lib.all lib.id (lib.zipListsWith (t: x: t.check x) ts xs);
         description = "tuple of" + lib.concatMapStrings (t: " (${t.description})") ts;
       };
       level = ints.unsigned;

@eclairevoyant
Copy link
Contributor Author

That's already fixed (#339084)

@anpin
Copy link
Contributor

anpin commented Sep 4, 2024

oh great, I guess it was not merged into unstable yet

@philiptaron philiptaron merged commit 271d117 into NixOS:master Sep 4, 2024
9 of 10 checks passed
@eclairevoyant eclairevoyant deleted the fix-nixos-lib branch September 4, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants