Skip to content

treewide: use mkTarget (batch 2)#1362

Merged
trueNAHO merged 31 commits intonix-community:masterfrom
0xda157:treewide-mkTarget-2
May 23, 2025
Merged

treewide: use mkTarget (batch 2)#1362
trueNAHO merged 31 commits intonix-community:masterfrom
0xda157:treewide-mkTarget-2

Conversation

@0xda157
Copy link
Copy Markdown
Contributor

@0xda157 0xda157 commented May 23, 2025

Things done

Notify maintainers

@stylix-automation stylix-automation bot added topic: nixos NixOS target topic: home-manager Home Manager target labels May 23, 2025
@0xda157 0xda157 force-pushed the treewide-mkTarget-2 branch from a698361 to 668e367 Compare May 23, 2025 00:01
@0xda157 0xda157 changed the title treewide: use mkTarget (part 2) treewide: use mkTarget (batch 2) May 23, 2025
@0xda157 0xda157 force-pushed the treewide-mkTarget-2 branch 7 times, most recently from 2c09de6 to b7555bf Compare May 23, 2025 02:34
@0xda157 0xda157 marked this pull request as ready for review May 23, 2025 02:42
@0xda157 0xda157 force-pushed the treewide-mkTarget-2 branch from b7555bf to ed85314 Compare May 23, 2025 02:48
@0xda157 0xda157 force-pushed the treewide-mkTarget-2 branch from ed85314 to f4ef0dc Compare May 23, 2025 15:13
Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

this should be rebased or merge committed, DO NOT SQUASH so any problematic commits can be reverted.

-- #1342

I did not think of that before merging the previous batch with a merge commit, but in this case, it might be better to apply these treewide mkTarget changes in one commit to prevent littering the commit history with noisy <SCOPE>: use mkTarget commits. Using smaller commits to "trivially" revert /modules/<SCOPE>/ changes is still trivial with git checkout <HASH_OF_THIS_SQUASH_MERGE>~ modules/<SCOPE>/. IMHO, it would be better to sqaush merge this PR.

Splitting this treewide patchset into smaller patchet would make more sense in much larger projects, like the Linux Kernel, where each patch would be reviewed by entirely different submodule maintainers. However, our project is far from being this big and the core maintainers essentially just review the entire PR as a whole. The /module/<SCOPE>/meta.nix module maintainers are also not really involved in these treewide changes, making this use case somewhat non-existent in our case.

@0xda157 0xda157 force-pushed the treewide-mkTarget-2 branch from f4ef0dc to 60b976e Compare May 23, 2025 16:36
@0xda157
Copy link
Copy Markdown
Contributor Author

0xda157 commented May 23, 2025

this should be rebased or merge committed, DO NOT SQUASH so any problematic commits can be reverted.
-- #1342

I did not think of that before merging the previous batch with a merge commit, but in this case, it might be better to apply these treewide mkTarget changes in one commit to prevent littering the commit history with noisy <SCOPE>: use mkTarget commits. Using smaller commits to "trivially" revert /modules/<SCOPE>/ changes is still trivial with git checkout <HASH_OF_THIS_SQUASH_MERGE>~ modules/<SCOPE>/. IMHO, it would be better to sqaush merge this PR.

Didn't realize that it's possible to revert edits to a specific file with git, in that case this should be squashed.

Copy link
Copy Markdown
Member

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

LGTM.

@trueNAHO trueNAHO merged commit 7ffb31d into nix-community:master May 23, 2025
5 checks passed
@stylix-automation
Copy link
Copy Markdown
Contributor

Successfully created backport PR for release-25.05:

stylix-automation bot pushed a commit that referenced this pull request May 23, 2025
Link: #1362

Reviewed-by: Flameopathic <64027365+Flameopathic@users.noreply.github.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit 7ffb31d)
@0xda157 0xda157 deleted the treewide-mkTarget-2 branch May 23, 2025 17:27
trueNAHO pushed a commit that referenced this pull request May 23, 2025
Link: #1362

Reviewed-by: Flameopathic <64027365+Flameopathic@users.noreply.github.com>
Reviewed-by: NAHO <90870942+trueNAHO@users.noreply.github.com>
(cherry picked from commit 7ffb31d)
@repparw
Copy link
Copy Markdown
Contributor

repparw commented May 23, 2025

this broke tofi. removed lib from {...}, still used in a couple calls. lib.toHexString, couple more

@repparw
Copy link
Copy Markdown
Contributor

repparw commented May 23, 2025

@trueNAHO maybe revert tofi on branches as it doesnt build? (or throw a commit adding lib back, ig, dont see another problem)

trueNAHO added a commit to trueNAHO/stylix that referenced this pull request May 23, 2025
Fixes: 7ffb31d ("treewide: use mkTarget (batch 2) (nix-community#1362)")
@trueNAHO
Copy link
Copy Markdown
Member

trueNAHO commented May 23, 2025

this broke tofi. removed lib from {...}, still used in a couple calls. lib.toHexString, couple more

@trueNAHO maybe revert tofi on branches as it doesnt build? (or throw a commit adding lib back, ig, dont see another problem)

@repparw, can you test #1370? That PR will be automatically merged after CI passes. Let me know if the issue is still not resolved after that.

trueNAHO added a commit that referenced this pull request May 23, 2025
Fixes: 7ffb31d ("treewide: use mkTarget (batch 2) (#1362)")
Link: #1370

Reviewed-by: awwpotato <awwpotato@voidq.com>
Tested-by: repparw <45952970+repparw@users.noreply.github.com>
stylix-automation bot pushed a commit that referenced this pull request May 23, 2025
Fixes: 7ffb31d ("treewide: use mkTarget (batch 2) (#1362)")
Link: #1370

Reviewed-by: awwpotato <awwpotato@voidq.com>
Tested-by: repparw <45952970+repparw@users.noreply.github.com>
(cherry picked from commit e4fde51)
trueNAHO added a commit that referenced this pull request May 23, 2025
Fixes: 7ffb31d ("treewide: use mkTarget (batch 2) (#1362)")
Link: #1370

Reviewed-by: awwpotato <awwpotato@voidq.com>
Tested-by: repparw <45952970+repparw@users.noreply.github.com>
(cherry picked from commit e4fde51)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: home-manager Home Manager target topic: nixos NixOS target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants