Skip to content

lzfse: modernize derivation #348766

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
Merged
tomodachi94 merged 1 commit into from
Nov 22, 2024
Merged

lzfse: modernize derivation #348766

tomodachi94 merged 1 commit into from
Nov 22, 2024

Conversation

@KSJ2000
Copy link
Contributor

@KSJ2000 KSJ2000 commented Oct 15, 2024

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.

@ofborg ofborg 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 Oct 15, 2024
HeitorAugustoLN
HeitorAugustoLN reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@HeitorAugustoLN HeitorAugustoLN left a comment

Choose a reason for hiding this comment

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

It would be better if each change would be made in a separate commit here, such as nixfmt, removing with lib, etc.

@KSJ2000
Copy link
Contributor Author

KSJ2000 commented Oct 17, 2024

It would be better if each change would be made in a separate commit here, such as nixfmt, removing with lib, etc.

nixfmt? I did no such thing. My alphabetization just happened to coincide; the package was already fmted.
Please point me to where it says with lib should be a separate commit, because by that format, replacing rec with finalAttrs should be a separate commit as well.

@HeitorAugustoLN
Copy link
Member

HeitorAugustoLN commented Oct 17, 2024

It would be better if each change would be made in a separate commit here, such as nixfmt, removing with lib, etc.

nixfmt? I did no such thing. My alphabetization just happened to coincide; the package was already fmted. Please point me to where it says with lib should be a separate commit, because by that format, replacing rec with finalAttrs should be a separate commit as well.

I thought you had formatted. But not necessarily it needs to be a separate commit, but it would be better if the commit explained everything that the commit does, and replace sha256 with hash is not the only thing being done. So it could have a message such as modernize derivation or something like that

nixpkgs/CONTRIBUTING.md

Line 497 in 73bd402

- Create a commit for each logical unit.

@KSJ2000 KSJ2000 changed the title lzfse: replace sha256 with hash lzfse: move to by-name and modernize derivation Oct 24, 2024
Aleksanaa
Aleksanaa reviewed Oct 31, 2024
View reviewed changes
@KSJ2000 KSJ2000 requested a review from Aleksanaa October 31, 2024 14:58
FliegendeWurst
FliegendeWurst approved these changes Nov 2, 2024
View reviewed changes
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

No rebuilds ✔️ .

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Nov 2, 2024
tomodachi94
tomodachi94 requested changes Nov 7, 2024
View reviewed changes
Copy link
Member

@tomodachi94 tomodachi94 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for adopting this package! Most of the diff looks okay, except you're missing from the maintainers list (see comment).

@Pandapip1 Pandapip1 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@Pandapip1
Copy link
Member

Pandapip1 commented Nov 9, 2024

Merge conflict due to #354531.

@KSJ2000 KSJ2000 marked this pull request as draft November 12, 2024 08:15
@KSJ2000 KSJ2000 marked this pull request as ready for review November 12, 2024 08:23
@KSJ2000 KSJ2000 changed the title lzfse: move to by-name and modernize derivation lzfse: modernize derivation Nov 12, 2024
@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label Nov 12, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 12, 2024
@KSJ2000 KSJ2000 marked this pull request as draft November 14, 2024 08:29
@KSJ2000 KSJ2000 marked this pull request as ready for review November 14, 2024 08:46
@KSJ2000 KSJ2000 mentioned this pull request Nov 14, 2024
Merged
13 tasks
Luflosi
Luflosi approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@Luflosi Luflosi left a comment

Choose a reason for hiding this comment

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

LGTM.

tomodachi94
tomodachi94 approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@tomodachi94 tomodachi94 left a comment

Choose a reason for hiding this comment

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

Lgtm, sorry for the wait ♥️

@tomodachi94 tomodachi94 added the 12.approvals: 3+ This PR was reviewed and approved by three or more persons. label Nov 22, 2024
@tomodachi94 tomodachi94 merged commit 964bdaa into NixOS:master Nov 22, 2024
14 of 15 checks passed
@KSJ2000 KSJ2000 deleted the lzfse branch November 22, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FliegendeWurst FliegendeWurst FliegendeWurst approved these changes

@HeitorAugustoLN HeitorAugustoLN HeitorAugustoLN approved these changes

@tomodachi94 tomodachi94 tomodachi94 approved these changes

@Aleksanaa Aleksanaa Awaiting requested review from Aleksanaa

+1 more reviewer

@Luflosi Luflosi Luflosi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@KSJ2000 @HeitorAugustoLN @Pandapip1 @FliegendeWurst @Luflosi @Aleksanaa @tomodachi94 @wegank