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/nixos: remove with lib; part 1 #335603

Merged
merged 139 commits into from
Aug 29, 2024

Conversation

Stunkymonkey
Copy link
Contributor

@Stunkymonkey Stunkymonkey commented Aug 18, 2024

Description of changes

part of #208242

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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: kernel The Linux kernel 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: printing 6.topic: julia 6.topic: flutter labels Aug 18, 2024
@Stunkymonkey Stunkymonkey changed the title treewide/nixos: remove with lib; treewide/nixos: remove with lib; part 1 Aug 18, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Aug 18, 2024
@Stunkymonkey
Copy link
Contributor Author

Stunkymonkey commented Aug 18, 2024

@philiptaron would you consider this "size" of MR ok to review?

I am going to split up the individual services.

(I have already written a shitty script to generate these changes, with a total of 1241 changed files. I will review all files by hand before I submit them via PR.)

@philiptaron
Copy link
Contributor

@Stunkymonkey: I think a few things 🙂

  1. I really like one commit per module, named for the module. That helps on many fronts, but especially rebuild and regression hunting.
  2. I like about ~150 max commits per PR. That's about my ability to review and say yes. In total, that would be about 8 or 9 PRs for the whole of the nixos/ directory. Totally manageable. Add me for all of it.
  3. I really want to see the tool that you're using, "shitty" or not. That helps me build an intuition for what sort of defects or otherwise to look out for.
  4. If there's a module with more than ~50 lines of changes in it, skip that for the bulk PRs.

Thank you for tackling this refactor!

@philiptaron philiptaron self-requested a review August 19, 2024 19:34
@Stunkymonkey
Copy link
Contributor Author

@philiptaron my "shitty" script (simple replace):

shitty-patch.sh
#!/usr/bin/env bash

# Get the list of files containing "with lib;"
files=$(grep -rlFx "with lib;" /home/felix/code/system-installs/nixpkgs/nixos/)

options=(
    "any"
    "attrByPath"
    "attrNames"
    "attrValues"
    "boolToString"
    "cli."
    "concatLists"
    "concatMap"
    "concatStrings"
    "const"
    "elem"
    "escape"
    "filter"
    "flatten"
    "flip"
    "foldAttrs"
    "foldl"
    "foldr"
    "generators."
    "getBin"
    "getDev"
    "getExe"
    "hasAttr"
    "hasPrefix"
    "hasSuffix"
    "imap1"
    "isAttrs"
    "isBool"
    "isDerivation"
    "isInt"
    "isList"
    "isPath"
    "isString"
    "last"
    "length"
    "lessThan"
    "lists"
    "listToAttrs"
    "literalExpression"
    "literalMD"
    "maintainers."
    "makeBinPath"
    "makeLibraryPath"
    "mapAttrs"
    "maybeEnv"
    "mergeOneOption"
    "mkAliasOptionModuleMD"
    "mkBefore"
    "mkDefault"
    "mkEnableOption"
    "mkForce"
    "mkIf"
    "mkImageMediaOverride"
    "mkMerge"
    "mkOption"
    "mkOrder"
    "mkOverride"
    "mkPackageOption"
    "mkRemovedOptionModule"
    "mkRenamedOptionModule"
    "nameValuePair"
    "optional"
    "recursiveUpdate"
    "removePrefix"
    "replaceStrings"
    "setPrio"
    "singleton"
    "sort"
    "stringAfter"
    "stringLength"
    "stringToCharacters"
    "substring"
    "subtractLists"
    "teams."
    "toList"
    "toUpper"
    "types."
    "unique"
    "versionAtLeast"
    "versionOlder"
    "zipAttrs"
    # "all" too many other instances
    # "partition" too many other instances
)

# 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")

        # 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/'"$escaped_str"'/'"$escaped_lib_str"'/g' "$file"
    done

    sed -i "s/with maintainers;/with lib.maintainers;/g" "$file"
    sed -i "s/with types;/with lib.types;/g" "$file"
    # revert wrong locations
    sed -i "s/mlib.any/many/g" "$file"
done

~150 max commits per PR

I guess you wanted to say about "lines of changes"?

I started with #336404

@philiptaron
Copy link
Contributor

No, I'm saying 150 commits (150 files) 😁

I feel quite adept at reviewing patterns.

@philiptaron
Copy link
Contributor

The tool sadly makes a few bugs -- see if you can evolve it a bit so it doesn't make this:

https://github.com/NixOS/nixpkgs/blob/c8503c7029fcbbac6a0f190b9580b6067e217c8b/nixos/modules/security/pam.nix#L1607-L1608

@github-actions github-actions bot added 6.topic: emacs Text editor 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab labels Aug 24, 2024
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 24, 2024
@Stunkymonkey Stunkymonkey force-pushed the treewide-nixos-remove-with-lib branch 3 times, most recently from 34f9a0c to 060641e Compare August 28, 2024 22:33
@philiptaron philiptaron merged commit 117f3ce into NixOS:master Aug 29, 2024
9 of 10 checks passed
@fufexan
Copy link
Contributor

fufexan commented Sep 1, 2024

3b7622d#diff-9bd6954b6c183d51a0dcbf1f540c61b89a2427e1d3ee7af1dccd98ca2c859279R118 This must've slipped. utils has no lib inside, and this broke my system rebuild.

@Aleksanaa
Copy link
Member

How do we currently ensure that the rest of the changes are correct and not just because others haven't used them yet?

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Sep 3, 2024

easiest way imo would be to fix the script and then rerun it before this PR to identify the differences. one problem is the missing \b at the end and missing \W at the beginning which ends up matching substrings.

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Sep 3, 2024

Okay I've started running it locally, but it seems to be quite slow, not sure when it'll finish.

bash was not the right language for this but I'm in too deep now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: emacs Text editor 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants