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: remove stdenv where not needed #110687

Merged
merged 1 commit into from Jan 25, 2021

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented Jan 24, 2021

Motivation for this change
  • Part of Deprecate use of stdenv.lib across Nixpkgs #108938
  • After replacing stdenv.lib with lib we ended up with almost 3000 files where stdenv is mentioned, but actually not used/needed.
  • This PR fixes this and there are 0 rebuilds.
  • EditorConfig fails with Error: Argument list too long - it's OK to ignore I guess
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@SuperSandro2000
Copy link
Member

How do we verify that we did not remove it accidentally from plugins that are not evaluated by ofborg like we did with the lib migration around 5 times or so?

@prusnak
Copy link
Member Author

prusnak commented Jan 24, 2021

How do we verify that we did not remove it accidentally from plugins that are not evaluated by ofborg like we did with the lib migration around 5 times or so?

I use the following sanity checks locally:

  • nix-env -qa --json --file . => should throw no error
  • nix-shell -p nixpkgs-review --run "nixpkgs-review wip" => should return no rebuilds

Any other suggestions welcome!

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 24, 2021

  • nix-env -qa --json --file . => should throw no error

Those files are not checked by that. It would require some changes to eval them.

Those are the files that where affected by the stdenv.lib -> lib change and got a fix:

f4d0aad
6de3a58
a1bb960
2f68b13
7742f1c
3c02d06
b20b9f9
ad2b6d9
601080d
f381793
5105bf4

I have no idea how to prevent that.

@prusnak prusnak requested review from siraben and removed request for stigtsp, mmahut, ttuegel and RaghavSood January 24, 2021 14:49
@siraben
Copy link
Member

siraben commented Jan 24, 2021

Could you elaborate how you performed the transformation? What scripts did you run?

@prusnak
Copy link
Member Author

prusnak commented Jan 24, 2021

Could you elaborate how you performed the transformation? What scripts did you run?

I used ripgrep to see how many times the word "stdenv" is mentioned in each nix file and filtered out the files which have stdenv only on one line:

rg -c stdenv | grep '\.nix:1$'

This found around 3000 files. Some of them were false positives, because they had something like the following:

{ stdenv, lib } : stdenv.mkDerivation {

These were easy to filter out via grep for stdenv.*stdenv and these were manually corrected.

Then I used sed to remove occurrences of stdenv, , , stdenv and { stdenv and used the eval scripts above to detect where I need to revert the change.

@siraben
Copy link
Member

siraben commented Jan 25, 2021

@prusnak please resolve the merge conflicts.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I appreciate the effort but I am affraid that we break plugins again

@siraben
Copy link
Member

siraben commented Jan 25, 2021

I appreciate the effort but I am affraid that we break plugins again

It seems unlikely to break plugins this time because the change is being performed on files with only one occurrence of stdenv (which cannot be a free variable). For breakage to happen like it did in stdenv.lib -> lib we need to have at least 2 occurrences, one in the input params and the rest in the body.

@Profpatsch
Copy link
Member

This sounds like a simple enough change; I wonder if we could use hnix to do a full “unused variable” pass in the future.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

assuming ofborg passes

@prusnak
Copy link
Member Author

prusnak commented Jan 25, 2021

The CI is green again (modulo EditorConfig check) - I need to rebase the PR couple of times a day to catch up with other changes, that's why ofborg is sometimes processing

Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Diff looks legit

Scrolled trough output from git diff --word-diff HEAD^.. | sort -u | grep -E '(\[-|{\+)'

@SuperSandro2000 SuperSandro2000 merged commit a979486 into NixOS:master Jan 25, 2021
@prusnak prusnak deleted the stdenv-cleanup branch January 25, 2021 17:47
danieldk added a commit to danieldk/nixpkgs that referenced this pull request Jan 26, 2021
Fix fallout of NixOS#110687 (generic.nix requires stdenv).
@danieldk
Copy link
Contributor

danieldk commented Jan 26, 2021

I appreciate the effort but I am affraid that we break plugins again

It seems unlikely to break plugins this time because the change is being performed on files with only one occurrence of stdenv (which cannot be a free variable). For breakage to happen like it did in stdenv.lib -> lib we need to have at least 2 occurrences, one in the input params and the rest in the body.

This is incorrect. If you have something like

{ foo, bar, baz, stdenv, ...}@args:

import ./quux.nix args {
  # stdenv is not used
}

and quux.nix requires the stdenv attr, then evaluation will fail when you remove stdenv argument attrs.

This PR broke emacs.pkgs.trivialBuild (see #110861) as a result. It's probably an edge case, but there may be other instances.

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.

None yet

7 participants