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

nixos-option: cleanup and linting #355748

Merged
merged 16 commits into from
Dec 10, 2024
Merged

nixos-option: cleanup and linting #355748

merged 16 commits into from
Dec 10, 2024

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Nov 13, 2024 •
edited
Loading

Things done

Depends on #355745

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

@Mic92 Mic92 marked this pull request as draft November 13, 2024 20:35
@Mic92 Mic92 mentioned this pull request Nov 13, 2024
Merged
13 tasks
@Mic92 Mic92 force-pushed the nixos-option-cleanup branch 2 times, most recently from b46c1d7 to 895c4ea Compare November 13, 2024 20:55
Mic92 added 8 commits November 13, 2024 23:10
@Mic92
nixos-option: add clang-tidy
00c3a3d
@Mic92
nixos-option: add devShell
7e9e33e
@Mic92
nixos-option: clean up includes based on clang-tidy suggestions
2269814 
Before the code was using include-what-you-means, but it's a bit tricky
to integrate this in nix. I had some trouble with nix-eval-jobs when using include-what-you-means.
clang-tidy on the other hand proved more robust to me and is useful for linting in general,
so less tooling overall for this little project.
@Mic92
nixos-option: use c++20 starts_with
553461a
@Mic92
nixos-option: explicitly cast to int from size_t
e8d76d6 
to make the linter happy.
@Mic92
nixos-option: don't throw errors in main
2cfcb6f 
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/exception-escape.html
@Mic92
nixos-option: enable bugprone lints
01580b1
@Mic92
nixos-option: enable performance lint
c3e4852
@Mic92 Mic92 force-pushed the nixos-option-cleanup branch from e2af8ab to 8086ffc Compare November 13, 2024 23:28
@Mic92 Mic92 marked this pull request as ready for review November 13, 2024 23:28
@Mic92
Copy link
Member Author

Mic92 commented Nov 13, 2024

cc @xokdvium

xokdvium
xokdvium approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

In general LGTM module the above comments and the noisy diff because of modernize-use-trailing-return-type. It's purely stylistic, but it's fine by me if that's intended.

pkgs/tools/nix/nixos-option/src/nixos-option.cc Outdated Show resolved Hide resolved
pkgs/tools/nix/nixos-option/.clang-tidy Show resolved Hide resolved
pkgs/tools/nix/nixos-option/src/nixos-option.cc Show resolved Hide resolved
pkgs/tools/nix/nixos-option/src/libnix-copy-paste.cc Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 14, 2024
@Mic92 Mic92 force-pushed the nixos-option-cleanup branch from 8086ffc to cf11719 Compare November 18, 2024 16:23
@Mic92 Mic92 requested a review from xokdvium November 18, 2024 16:32
lf-
lf- previously requested changes Nov 18, 2024
View reviewed changes
Copy link
Member

@lf- lf- left a comment

Choose a reason for hiding this comment

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

I am opposed since I want nixos-option gone as a going concern, rather than increased in complexity/maintenance-level.

Here is a PR implementing that: #313497

@Mic92
Copy link
Member Author

Mic92 commented Nov 18, 2024 •
edited
Loading

I am opposed since I want nixos-option gone as a going concern, rather than increased in complexity/maintenance-level.

Looks like the other pull request is still not finished as well. What's you ETA on that?
It should be also easier to support flakes with this approach btw. No need to fiddle with builtins.getFlake.

@lf-
Copy link
Member

lf- commented Nov 19, 2024

It should be also easier to support flakes with this approach btw. No need to fiddle with builtins.getFlake.

Right, but builtins.getFlake being broken garbage (due to a shockingly small number of actual bugs; it really mostly is the way it handles git repos being completely different to the CLI) is a huge ecosystem-wide tooling problem that makes every tool including colmena and every other tool that does not link to Nix busted for flakes. It is much more worthwhile to fix that.

Unrelatedly, nixos-option has been causing wasted closure size and annoyance to Lix users since the inception of Lix (and wasted closure size for CppNix users for even longer because nixos-option is unmaintained and usually pinned to some old CppNix version it builds with), while being fragile code by virtue of linking to the CppNix API and being maintained in nixpkgs where the tooling support for hosting a C++ project is not great and where there are very few maintainers for in-tree C++, which is why it is in the poor state you found it in.

I am saying this as the person who has done multiple fixes to nixos-option because they needed doing, including the one that was urgently done so that Nix 2.16 (or something like that) could be properly dropped in January when the fod corruption security bug happened.

The C++ nixos-option just needs to be eliminated. It is a maintenance liability. Adding linting or improving the build system is not going to dodge the root cause which is that it is linking to a Nix's unstable C++ API and being C++ code in the nixpkgs tree to begin with, something which will never change without it being rewritten to use the CLI.

Looks like the other pull request is still not finished as well. What's you ETA on that?

I don't know. I know that it was at one point in a perfectly mergeable state right before flakes support was being added, introducing a bunch of lack of clarity as to what was the right approach. It could probably have the flake support removed in an hour of concerted effort and be simply merged.

@Mic92
Copy link
Member Author

Mic92 commented Nov 19, 2024

I believe nothing in this pull request complicates the completion of the other pull request. Therefore, I don't see a strong justification for delaying these maintenance fixes to the current implementation while waiting for a rewrite that hasn't yet been widely tested.

If it's really just a matter of an hour, I would suggest just finishing it, especially since the actual reason for you being against it is the dependency of nix rather the content of this pull request.

With that in mind, I propose moving forward with merging this pull request within the next two weeks unless there are strong objections from NixOS maintainer or the other pull requests gets merged first.

cc @NixOS/nix-team

tomberek
tomberek approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Looks like it is mostly style and standardizing the code. No objections. Thanks!

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 19, 2024
@tomberek tomberek dismissed lf-’s stale review December 10, 2024 03:30

As noted, several weeks have passed. Merging.

@tomberek tomberek merged commit 1ea7633 into NixOS:master Dec 10, 2024
29 checks passed
@Mic92 Mic92 deleted the nixos-option-cleanup branch December 10, 2024 04:27
@K900
Copy link
Contributor

K900 commented Dec 10, 2024

https://hydra.nixos.org/build/281786140/nixlog/54/tail

@K900 K900 mentioned this pull request Dec 10, 2024
Merged
@Mic92
Copy link
Member Author

Mic92 commented Dec 10, 2024

This links the tests for nixos-option so it's more obvious what to tests: #363967

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xokdvium xokdvium xokdvium approved these changes

@tomberek tomberek tomberek approved these changes

@lf- lf- lf- left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Mic92 @lf- @K900 @tomberek @xokdvium @wegank