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: replace stdenv.is with stdenv.hostPlatform.is #341407

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Sep 12, 2024

Approve or thumbs up to show that you approve of the idea, or request changes on the first file to discuss why you don't think it's a good idea.

Join https://matrix.to/#/#stdenv:nixos.org to discuss

I'll reset to master and rerun the commands before merge.
Further replacements can be done in the PR where the warnings are added.
The commits can be added to .git-blame-ignore-revs in a separate PR

treewide: replace stdenv.is with stdenv.hostPlatform.is

In preparation for the deprecation of stdenv.isX.

These shorthands are not conducive to cross-compilation because they
hide the platforms.

Darwin might get cross-compilation for which the continued usage of stdenv.isDarwin will get in the way

One example of why this is bad and especially affects compiler packages
#343059

There are too many files to go through manually but a treewide should
get users thinking when they see a hostPlatform.isX in a place where it
doesn't make sense.

fd --type f "\.nix" | xargs sd --fixed-strings "stdenv.is" "stdenv.hostPlatform.is"
fd --type f "\.nix" | xargs sd --fixed-strings "stdenv'.is" "stdenv'.hostPlatform.is"
fd --type f "\.nix" | xargs sd --fixed-strings "clangStdenv.is" "clangStdenv.hostPlatform.is"
fd --type f "\.nix" | xargs sd --fixed-strings "gccStdenv.is" "gccStdenv.hostPlatform.is"
fd --type f "\.nix" | xargs sd --fixed-strings "stdenvNoCC.is" "stdenvNoCC.hostPlatform.is"
fd --type f "\.nix" | xargs sd --fixed-strings "inherit (stdenv) is" "inherit (stdenv.hostPlatform) is"
fd --type f "\.nix" | xargs sd  "buildStdenv.is" "buildStdenv.hostPlatform.is"
fd --type f "\.nix" | xargs sd  "effectiveStdenv.is" "effectiveStdenv.hostPlatform.is"
fd --type f "\.nix" | xargs sd  "originalStdenv.is" "originalStdenv.hostPlatform.is"

The rebuilds are due to src ./. or similar

image

https://gist.github.com/Artturin/4cc1e73fc612cb7a79459a9bb6bc27a3

@Artturin
Copy link
Member Author

Undrafting to get reviews from probably all the codeowners in nixpkgs.

To keep the emails to a minimum either approve or thumbs up the PR if the idea is okay, or if it needs changes then let's discuss in https://matrix.to/#/#stdenv:nixos.org

@K900
Copy link
Contributor

K900 commented Sep 19, 2024

Idea is OK, list of merge conflicts is scary. Maybe do this stepwise?

@@ -32,7 +32,7 @@ pkgs.runCommand "nixpkgs-lib-tests-nix-${nix.version}" {
nativeBuildInputs = [
nix
pkgs.gitMinimal
] ++ lib.optional pkgs.stdenv.isLinux pkgs.inotify-tools;
] ++ lib.optional pkgs.stdenv.hostPlatform.isLinux pkgs.inotify-tools;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
] ++ lib.optional pkgs.stdenv.hostPlatform.isLinux pkgs.inotify-tools;
] ++ lib.optional pkgs.stdenv.buildPlatform.isLinux pkgs.inotify-tools;

This probably applies to most conditionals for any nativeBuildInputs.

@Artturin Artturin added the 6.topic: stdenv Standard environment label Sep 19, 2024
@github-actions github-actions bot removed the 6.topic: stdenv Standard environment label Sep 19, 2024
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.

7 participants