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: nixfmt tree #376179

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

Conversation

RossComputerGuy
Copy link
Member

Things done

nixfmt the LLVM nix tree as a part of the pre-work for #375431.

  • 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.

@RossComputerGuy RossComputerGuy marked this pull request as ready for review January 23, 2025 17:46
@github-actions github-actions bot added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Jan 23, 2025
@github-actions github-actions 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 labels Jan 23, 2025
@alyssais
Copy link
Member

Ack.

1 similar comment
@pwaller
Copy link
Contributor

pwaller commented Jan 23, 2025

Ack.

@RossComputerGuy
Copy link
Member Author

What's with the ack? I think it's better to just bite the reformatting than wait for a hit from a treewide reformatting PR. Plus, this'll make it easier to deal with some of the next PR's I'm planning on.

@alyssais
Copy link
Member

"Ack" is short for "acknowledged" (positively, but not reviewed). See e.g. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by for a detailed explanation of what an ack is in an open source review context.

@emilazy
Copy link
Member

emilazy commented Jan 24, 2025

Please add this to .git-blame-ignore-revs to not break git blame. Note that at least one significant change to the formatting is planned and there will be a treewide within the next month or two that formats these anyway (that will modify them regardless due to the planned change) so it may be best just to wait.

@pwaller
Copy link
Contributor

pwaller commented Jan 24, 2025

it may be best just to wait.

An alternate view. Fixing the llvm tree doesn't need to be conditioned on whether the rest of the nixpkgs tree is being fixed. If anything it would surely suit the llvm tree to do it at a time convenient for the llvm tree.

If someone starts a PR now which takes time to land; there is a fair chance it ends up having to be rebased through these changes. This could take up some precious cycles from everyone involved in those reviews.

I percieve that there is (relatively) little in flight right now in the llvm tree. I'd always like to minimize the amount of work that gets invalidated. I would push for landing whatever is in flight (and has a good chance of landing without major changes) now as quickly as we can, then land this.

@emilazy
Copy link
Member

emilazy commented Jan 24, 2025

It won’t be fixed insofar as “not needing another format again” because of the upcoming change to the formatting rules I mentioned. It does mean another entry in .git-blame-ignore-revs for all eternity, but that’s not the end of the world. The nature of the upcoming change means that doing it now might make git blame permanently worse, however, as lines are drastically rearranged and then rearranged back; .git-blame-ignore-revs isn’t perfect (though it’s possible it can handle that case okay; I haven’t tested).

This would also need doing on the 24.11 branch (and that commit adding to .git-blame-ignore-revs on both branches), otherwise backports will be a nightmare. Any pending changes on staging will also cause conflicts unless dealt with. That’s another thing the treewide will handle automatically.

In‐flight work should not pose a problem because of things like #363759 and NixOS/nixfmt#277 (which will have had any kinks ironed out by the time of the treewide format, so it may actually cause less pain to wait).

Anyway, not a hard objection as long as the commit is added to .git-blame-ignore-revs and the branches are handled properly. I’m just saying that the upside is relatively slight and the downsides may outweigh it. Getting the details right to make the process is not a pain is why the first treewide didn’t happen many months ago and why the second is still pending on the one (IIRC) remaining blocker.

@pwaller
Copy link
Contributor

pwaller commented Jan 24, 2025

Your comment was persuasive and informative. I didn't realise the formatting rules are changing. If so, I agree we should wait for that to settle.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants