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

llvmPackages_{12,13,14,15,16,17,18,19,git}: commonify patches #333521

Merged
merged 10 commits into from
Aug 16, 2024

Conversation

RossComputerGuy
Copy link
Member

Description of changes

Reduce the number of patches we have by finding duplicates. Modify getVersionFile in common/default.nix to branch to min/max version patches so we only have to keep 1 copy of a specific version of a patch.

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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 Aug 9, 2024
@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-patch-common branch 2 times, most recently from 35a04f0 to 721aaa8 Compare August 9, 2024 21:38
LLVM 14's clang purity patch was removed as it is a duplicate of 12's.
@RossComputerGuy
Copy link
Member Author

Swift build failures can be ignored as they are happening on Hydra as well. It looks like it at least passes the patchPhase.

@RossComputerGuy RossComputerGuy marked this pull request as ready for review August 10, 2024 00:18
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.

This is fantastic work — look at all those deletions! Thank you!

I'm just wondering if there's a better way to express it…

@@ -74,7 +74,98 @@ let
p:
builtins.path {
name = builtins.baseNameOf p;
path = "${metadata.versionDir}/${p}";
path =
Copy link
Member

Choose a reason for hiding this comment

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

This big nested conditional is not great — I think maybe it indicates that getVersionFile is not the right abstraction. Maybe it'd be better to just have a list of patches for each major LLVM version?

Copy link
Member Author

@RossComputerGuy RossComputerGuy Aug 12, 2024

Choose a reason for hiding this comment

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

Maybe but there's conditional patches and some patches are the same between versions while having different siblings. I was thinking of doing an attr set and have it work like a table but I found it to be simpler to do it this way.

I was thinking of doing a PR to clean up the common default.nix so it wouldn't be as sloppy. Probably in that one, I could have an attr set of versions and values which could be a string or null which then matches into figuring out where that patch will be.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of doing an attr set and have it work like a table

This is what I was thinking of. Was the problem just that you'd have to repeat the patch of a patch multiple times to say that different versions have the same patch? If so, I think that would be easier to understand overall.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, I'm not sure.

@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-patch-common branch 4 times, most recently from 95c874f to 521356c Compare August 13, 2024 04:21
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.

This is still very complicated, but it's better, and maybe there's just not a better way (apart from upstreaming the patches, of course).

Collapsing the attrsets into single lines (if that's allowed by formatting rules) might help, by making it a bit denser.

@RossComputerGuy
Copy link
Member Author

Collapsing the attrsets into single lines (if that's allowed by formatting rules) might help, by making it a bit denser.

Yeah, I originally did that but the formatter changed it.

@RossComputerGuy
Copy link
Member Author

Before we merge, I want to be able to reproduce and fix that CI error. However, I tried and couldn't get it to fail locally.

@RossComputerGuy
Copy link
Member Author

Well, by-name passed so we should be gtg now.

path = ../13;
}
];
"compiler-rt/gnu-install-dirs.patch" = [
Copy link
Contributor

Choose a reason for hiding this comment

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

possible error here?

@tomberek tomberek merged commit 94574c7 into NixOS:master Aug 16, 2024
20 of 22 checks passed
@RossComputerGuy RossComputerGuy deleted the feat/llvm-patch-common branch August 16, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants