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

clang-tools: move into llvmPackages #191698

Merged
merged 4 commits into from
Jun 22, 2024

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Sep 17, 2022

Description of changes

Format the Nix expression of clang-tools using nixfmt, in accordance with NixOS/rfcs#166.

Move the The clang-tools expression folder under llvm/common and clang-tools package under llvmPackages.

Move clang-tools_<version> into aliases.nix, specifying as llvmPackages_<version>.clang-tools.

Provide clang-tools-python that links and wraps executables from clang-unwrapped.python output. If applied, users will be able to enjoy git clang-format with clang-tools-python installed (or introduced inside Nix shell).

As for "${clang-unwrapped.python}/bin/scan-view", it depends on the relative module inside "../share". The dependent module of scan-view, ScanView.py, is currently not moved into clang-unwrapped.python, which will be fixed by the staging PR #191801.

Cc:
@Patryk27 clang-tools maintainer

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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 (The 24 updated packages will take some time to build.)
  • Tested basic functionality of all binary files (usually in ./result/bin/) (Tested git-clang-format, but not scan-view)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ShamrockLee
Copy link
Contributor Author

@ofborg build bat-extras.prettybat cloudcompare entwine grass libpulsar nominatim pdal python310Packages.tiledb python39Packages.tiledb qgis qgis-ltr tiledb vscode-extensions.ms-vscode-cpptools

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Sep 18, 2022

Just seperate those Python scripts to a new output python, so that packages depending on clang-tools won't be forced to have Python in their closures.

It will still be installed as meta.outputsToInstall are set to include "python".

Thank you for your time and patience.

@ofborg ofborg bot requested a review from Patryk27 September 18, 2022 13:40
Copy link
Member

@Patryk27 Patryk27 left a comment

Choose a reason for hiding this comment

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

Looks pretty neat 🙂

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 18, 2022
@ofborg ofborg bot requested a review from Patryk27 September 22, 2022 18:50
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Sep 22, 2022
@ShamrockLee
Copy link
Contributor Author

Could it be merged now?

Or should I split the wrapped python script into a new derivation clang-tools-python to reduce the number of rebuild?

@Patryk27
Copy link
Member

The changes look fine to me -- I don't personally have merging rights, though 👀

@ShamrockLee
Copy link
Contributor Author

No wonder. I had thought that all the members have the merging right.

@ShamrockLee ShamrockLee changed the title clang-tools: provide "${clang-unwrapped.python}/bin/*" executables clang-tools-python: init Dec 2, 2022
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 2, 2022
@ShamrockLee
Copy link
Contributor Author

Just make a new derivation clang-tools-python instead. This way, packages depending on clang-tools won't have to rebuild whenever python gets rebuild.

Wrapping the Python scripts into a new package also guarantees zero rebuild of the current packages, and is thus easier to backport.

Copy link
Member

@Patryk27 Patryk27 left a comment

Choose a reason for hiding this comment

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

🚀

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1507

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/development/tools/clang-tools/python.nix Outdated Show resolved Hide resolved
pkgs/development/tools/clang-tools/python.nix Outdated Show resolved Hide resolved
pkgs/development/tools/clang-tools/python.nix Outdated Show resolved Hide resolved
@ShamrockLee ShamrockLee force-pushed the clang-tools-python branch 2 times, most recently from 6f5a41c to 95e28fa Compare December 9, 2022 20:50
@ShamrockLee ShamrockLee requested review from SuperSandro2000 and removed request for matthewbauer December 9, 2022 20:57
@ofborg ofborg bot requested a review from Patryk27 May 28, 2024 07:13
@rrbutani
Copy link
Contributor

Not sure if the change of <pkg>.override interfaces caused by moving clang-tools into llvmPakcages would also be considered backward-incompatible.

oh, good point! here, let's ask:


@wegank This PR changes some top-level attrs to be aliases and changes some package attributes such that existing clang-tools_<ver>.override { ... } invocations may break (now takes clang, libcxxClang instead of llvmPackages). We're wondering if this is okay to backport to 24.05 or if this is considered a breaking change.

@wegank
Copy link
Member

wegank commented May 28, 2024

I'd consider the change of argument for clang-tools to be breaking. There's also the move to aliases for clang-tools_1{2-8}, which sounds breaking too.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 29, 2024
Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

@ShamrockLee thanks for working on this; one last minor change and this will be good to merge:

nixos/doc/manual/release-notes/rl-2405.section.md Outdated Show resolved Hide resolved
@rrbutani rrbutani removed the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label May 30, 2024
@rrbutani rrbutani added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 30, 2024
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS and removed 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related labels Jun 17, 2024
@ShamrockLee
Copy link
Contributor Author

I just rebased the feature branch on top of the master branch and resolved the merge conflict of the release note entries.

Is there anything I could help to push it forward?

@ShamrockLee
Copy link
Contributor Author

I simplified the package calling of clang-tools in each llvmPackages and fixed the evaluation, i.e.

     # Wrapper for standalone command line utilities
-    clang-tools = callPackage ../common/clang-tools {
-      inherit (tools) clang-unwrapped clang libcxxClang;
-      inherit llvm_meta;
-    };
+    clang-tools = callPackage ../common/clang-tools { };

Format default.nix with nixfmt in accordance with Nix RFC 166.

Manually Place the comments above the corresponding argument.
Add 24.11 release note entry about moving clang-tools into llvmPackages
and making clang-tools_<version> aliases.
@JohnRTitor
Copy link
Contributor

JohnRTitor commented Jun 22, 2024

Ok, I rebased it, lets merge this one after CI passes. It's been long due already.

@JohnRTitor JohnRTitor added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jun 22, 2024
@JohnRTitor
Copy link
Contributor

Result of nixpkgs-review pr 191698 run on x86_64-linux 1

1 package blacklisted:
  • nixos-install-tools
6 packages built:
  • llvmPackages_12.clang-tools
  • llvmPackages_13.clang-tools
  • llvmPackages_14.clang-tools
  • llvmPackages_15.clang-tools
  • llvmPackages_16.clang-tools
  • llvmPackages_18.clang-tools

@ofborg ofborg bot requested a review from Patryk27 June 22, 2024 20:38
@JohnRTitor JohnRTitor merged commit 6498422 into NixOS:master Jun 22, 2024
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants