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.

Sign up for GitHub

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

llvmPackages.lldbPlugins: drop llvm/clang rebuild #366401

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

llvmPackages.lldbPlugins: drop llvm/clang rebuild #366401

wants to merge 1 commit into from

Conversation

paparodeo
Copy link
Contributor

@paparodeo paparodeo commented Dec 19, 2024 •
edited
Loading

can add backport release-24.11 tag

noticed that llvm/clang were getting built 3 times rather than 2 when testing #366057 nix-build -A llvmPackages on staging and it is due to lldbPlugins scope. The one package in the atterset is a python wrapper around lldb with no buildPhase.

Eliminate rebuilding the following packages on darwin just for this attrset:

< DarwinTools-1.drv
< DarwinTools-1.tar.gz.drv
< IOKit-11.0.drv
< builder.pl.drv
< clang-16.0.6.drv
< clang-at-least-16-LLVMgold-path.patch.drv
< clang-darwin-An-OS-version-preprocessor-define.patch.drv
< clang-src-16.0.6.drv
< llvm-16.0.6.drv
< llvm-src-16.0.6.drv
< psutil-6.0.0.tar.gz.drv
< python3-3.12.7-env.drv
< python3.12-psutil-6.0.0.drv
< sysctl-system_cmds-1012.drv

found by diffing the output of the command below before and after this change:

nix-store --query -R $(nix-instantiate -A llvmPackages.lldbPlugins) | \
  cut -d- -f 2- | sort

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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 the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Dec 19, 2024
@paparodeo paparodeo marked this pull request as draft December 19, 2024 02:40
@paparodeo
llvmPackages.lldbPlugins: drop llvm/clang rebuild
1259856 
Eliminate rebuilding the following packages on darwin just for this
attrset:

< DarwinTools-1.drv
< DarwinTools-1.tar.gz.drv
< IOKit-11.0.drv
< builder.pl.drv
< clang-16.0.6.drv
< clang-at-least-16-LLVMgold-path.patch.drv
< clang-darwin-An-OS-version-preprocessor-define.patch.drv
< clang-src-16.0.6.drv
< llvm-16.0.6.drv
< llvm-src-16.0.6.drv
< psutil-6.0.0.tar.gz.drv
< python3-3.12.7-env.drv
< python3.12-psutil-6.0.0.drv
< sysctl-system_cmds-1012.drv

found by diffing the output of the command below before and after this
change:
```
nix-store --query -R $(nix-instantiate -A llvmPackages.lldbPlugins) | \
  cut -d- -f 2- | sort
```
@paparodeo paparodeo force-pushed the lldbPlugins-no-llvm-rebuild branch from 66a28a0 to 1259856 Compare December 19, 2024 02:48
@paparodeo paparodeo marked this pull request as ready for review December 19, 2024 02:49
@github-actions github-actions bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 labels Dec 19, 2024
@pwaller
Copy link
Contributor

pwaller commented Dec 21, 2024

Source changes look reasonable. Tried to reproduce, but not seeing the intended effect? I'm on x86_64-linux but passing --system aarch64-darwin to nix-instantiate.

$  gh pr checkout paparodeo:lldbPlugins-no-llvm-rebuild
Previous HEAD position was 087408a40744 couchdb3: 3.4.1 -> 3.4.2 (#361961)
Switched to branch 'lldbPlugins-no-llvm-rebuild'

$ nix-store --query -R $(nix-instantiate --system aarch64-darwin -A llvmPackages_19) | wc -l
2189

$ git checkout HEAD~1
HEAD is now at 087408a40744 couchdb3: 3.4.1 -> 3.4.2 (#361961)

$ nix-store --query -R $(nix-instantiate --system aarch64-darwin -A llvmPackages_19) | wc -l
2189

(i.e. derivation closure size is the same before and after the change)

@paparodeo
Copy link
Contributor Author

paparodeo commented Dec 21, 2024 •
edited
Loading

Source changes look reasonable. Tried to reproduce, but not seeing the intended effect? I'm on x86_64-linux but passing --system aarch64-darwin to nix-instantiate.

that shouldn't be an issue -- I can reproduce the same numbers when running on darwin or linux but I can't reproduce the ones you listed. [edit -- need to be using llvmPackages or llvmPackages_16 (whatever the default for llvm is)]

$ uname -os
Linux GNU/Linux
$ git switch lldbPlugins-no-llvm-rebuild
Switched to branch 'lldbPlugins-no-llvm-rebuild'
Your branch is up to date with 'origin/lldbPlugins-no-llvm-rebuild'.
$ nix-store --query -R $(nix-instantiate -A llvmPackages.lldbPlugins --system aarch64-darwin) | wc -l
1185
$ git checkout HEAD~1
HEAD is now at 087408a40744 couchdb3: 3.4.1 -> 3.4.2 (#361961)
$ nix-store --query -R $(nix-instantiate -A llvmPackages.lldbPlugins --system aarch64-darwin) | wc -l 
1199

@paparodeo
Copy link
Contributor Author

Source changes look reasonable. Tried to reproduce, but not seeing the intended effect? I'm on x86_64-linux but passing --system aarch64-darwin to nix-instantiate.

that shouldn't be an issue -- I can reproduce the same numbers when running on darwin or linux but I can't reproduce the ones you listed.

oh -- I see the issue. you're using llvmPackages_19 and you need to just use llvmPackages which maps to llvmPackages_16 which is what is used for the stdenv on master. this only effects darwin using the default llvmPackages the other ones are unaffected.

@pwaller
Copy link
Contributor

pwaller commented Dec 21, 2024

Ah, OK; I do see one fewer rebuild in that case. So, still two rebuilds of llvm-16.0.6 and the sources. Looks good to me if that is what you're expecting.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwaller pwaller pwaller approved these changes

@alyssais alyssais Awaiting requested review from alyssais

@dtzWill dtzWill Awaiting requested review from dtzWill

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@lovek323 lovek323 Awaiting requested review from lovek323

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

@rrbutani rrbutani Awaiting requested review from rrbutani

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

Assignees
No one assigned
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 1-10 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paparodeo @pwaller @wegank