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

Jump to bottom

toplevel: stdenv.lib -> lib #108934

Merged
merged 1 commit into from Jan 10, 2021
Merged

toplevel: stdenv.lib -> lib #108934

merged 1 commit into from Jan 10, 2021

Conversation

Profpatsch
Copy link
Member

The library does not depend on stdenv, that stdenv exposes lib is
an artifact of the ancient origins of nixpkgs.

Motivation for this change
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.

alyssais
alyssais approved these changes Jan 10, 2021
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

LGTM as long as OfBorg identifies it as rebuild-0.

siraben
siraben approved these changes Jan 10, 2021
View reviewed changes
Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

LGTM

@Atemu
Copy link
Member

Atemu commented Jan 10, 2021

Should such a change be done treewide whenever you get the chance to?

@FRidh
Copy link
Member

FRidh commented Jan 10, 2021

Are we in any way fixing anything by this or will this just cause work for people that have expressions relying on this?

@Profpatsch
Copy link
Member Author

or will this just cause work for people that have expressions relying on this?

How do you mean expressions relying on this?

@FRidh
Copy link
Member

FRidh commented Jan 10, 2021

How do you mean expressions relying on this?

Nevermind, I thought the change in stdenv was included here. This here in Nixpkgs is good.

@Atemu
Copy link
Member

Atemu commented Jan 10, 2021

Doesn't eval. Fix: Profpatsch#3

With that patch, nixpkgs-review says it doesn't change anything.

@siraben
Copy link
Member

siraben commented Jan 10, 2021

Should such a change be done treewide whenever you get the chance to?

The end goal is to completely remove stdenv.lib. When we perform it treewide it would make sense to split it into several smaller PRs.

@alyssais
Copy link
Member

The end goal is to completely remove stdenv.lib. When we perform it treewide it would make sense to split it into several smaller PRs.

I’m not sure we necessarily want to completely remove it, because there’ll be lots of external code that uses it. But we don’t want to be using it within Nixpkgs.

@Profpatsch
Copy link
Member Author

yeah, just not use it, maybe add a warning eventually, but that’s far out.

If we give people less examples, they won’t copy it around so much.

@siraben siraben mentioned this pull request Jan 10, 2021
Closed
@Profpatsch
Copy link
Member Author

@GrahamcOfBorg build dotnetPackages emscriptenPackages haxePackages luaPackages pythonPackages

@Profpatsch
toplevel: stdenv.lib -> lib
86b122c 
The library does not depend on stdenv, that `stdenv` exposes `lib` is
an artifact of the ancient origins of nixpkgs.
@Profpatsch Profpatsch merged commit 2497553 into NixOS:master Jan 10, 2021
@Profpatsch Profpatsch deleted the toplevel-stdenv-lib branch January 10, 2021 20:52
@bobby285271 bobby285271 mentioned this pull request Sep 19, 2021
Merged
jmercouris added a commit to atlas-engineer/nyxt that referenced this pull request Feb 2, 2022
@jmercouris
shell.nix: update to remove stdenv.lib -> lib
aa77727 
NixOS/nixpkgs#108934
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehmry ehmry ehmry approved these changes

@alyssais alyssais alyssais approved these changes

@siraben siraben siraben approved these changes

@FRidh FRidh Awaiting requested review from FRidh FRidh is a code owner

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
6.topic: lua 6.topic: python 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Profpatsch @Atemu @FRidh @siraben @alyssais @ehmry