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: {target->host}Platform non-compiler-ish packages #267229

Merged
1 commit merged into from Nov 17, 2023
Merged

treewide: {target->host}Platform non-compiler-ish packages #267229

1 commit merged into from Nov 17, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 13, 2023

This causes no rebuilds; eval (at least for the hydra jobset) is unaffected.

Description of changes

stdenv.targetPlatform really shouldn't be used by software that doesn't read, write, or manipulate executables.

I reviewed all uses of targetPlatform outside of pkgs/development/compilers and pkgs/stdenv, and replaced those which weren't involved in something which fits these criteria. Nearly all of them appear to have arisen from confusion about the meaning of host and target.

Findings

Here are the categories covering all the legitimate uses of stdenv.targetPlatform that I was able to find:

  • assemblers and compilers (but not interpreters or JITs) which emit machine code
  • debuggers and fuzzers (host!=target useful only if host.canExecute target)
  • tools for manipulating executables (bintools, patchelf)
  • emulators that need target binaries/firmware/bootloaders at emulator build time
  • awkward exceptions:
    • gobject-introspection (technically a compiler, but does not emit machine code)
    • software which uses target-platform paths at build time
      • pkg-config (probably the only legit example)
      • s6-rc-compile, s6-linux-init (I may try to upstream a fix for this)
      • a few poorly-designed build managers

After this PR, there are only 65 files in pkgs but outside of pkgs/development/compilers which mention targetPlatform. Filtering out pkgs/development/tools/build-managers/, pkgs/top-level/, pkgs/stdenv, and pkgs/build-support brings the number down to 35.

Motivation

This PR is part of work towards a theory that the vast majority of nixpkgs ought to be oblivious to targetPlatform, and that we will all be happier that way.

@ghost ghost changed the title treewide: s_targetPlatform_hostPlatform_ in non-compiler packages treewide: replace targetPlatform->hostPlatform in non-compiler packages Nov 13, 2023
@ghost ghost changed the title treewide: replace targetPlatform->hostPlatform in non-compiler packages treewide: targetPlatform->hostPlatform for non-compiler-ish packages Nov 13, 2023
@ghost ghost changed the title treewide: targetPlatform->hostPlatform for non-compiler-ish packages treewide: targetPlatform->hostPlatform non-compiler-ish packages Nov 13, 2023
@ghost ghost changed the title treewide: targetPlatform->hostPlatform non-compiler-ish packages treewide: {target->host}Platform non-compiler-ish packages Nov 13, 2023
@Artturin
Copy link
Member

Artturin commented Nov 13, 2023

gobject-introspection compiles code, but via an emulator because it's not a cross-compiler 😩

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-darwin: 11-100 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Nov 13, 2023
@ghost ghost marked this pull request as ready for review November 14, 2023 01:37
@ghost ghost requested a review from adisbladis as a code owner November 14, 2023 01:37
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 16, 2023
stdenv.targetPlatform really shouldn't be used by software that
doesn't generate or manipulate binaries.  I reviewed all uses of
targetPlatform outside of pkgs/development/compilers and pkgs/stdenv
and replaced those which weren't involved in something which fits
these criteria.
@ghost ghost marked this pull request as draft November 17, 2023 04:17
@ghost ghost marked this pull request as ready for review November 17, 2023 04:17
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 17, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 17, 2023
@ghost
Copy link
Author

ghost commented Nov 17, 2023

  • This is a treewide PR at high risk of accruing merge conflicts.
  • This PR has one non-author approval.
  • Ofborg has verified that this causes zero rebuilds.
    • I have also confirmed that it does not affect eval for pkgs.tests.cross.sanity

Considering all of the above, I am going to self-merge this PR.

@ghost ghost merged commit c7e0f6b into NixOS:master Nov 17, 2023
@ghost ghost deleted the pr/treewide/targetPlatform branch November 17, 2023 08:07
This pull request was closed.
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: hygiene 6.topic: printing 6.topic: python 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants