Skip to content

stash: refactor; 0.25.1 -> 0.27.2; nixos/stash: init#323231

Merged
FliegendeWurst merged 4 commits intoNixOS:masterfrom
DrakeTDL-Forks:stash
Jan 26, 2025
Merged

stash: refactor; 0.25.1 -> 0.27.2; nixos/stash: init#323231
FliegendeWurst merged 4 commits intoNixOS:masterfrom
DrakeTDL-Forks:stash

Conversation

@DrakeTDL
Copy link
Member

@DrakeTDL DrakeTDL commented Jun 28, 2024

Description of changes

This Pull Request:

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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Jun 28, 2024
@DrakeTDL DrakeTDL force-pushed the stash branch 7 times, most recently from 1e88836 to 42d1ca5 Compare June 29, 2024 07:37
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 29, 2024
@DrakeTDL DrakeTDL force-pushed the stash branch 3 times, most recently from 4206b75 to 1d0a178 Compare June 30, 2024 05:16
@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests labels Jul 1, 2024
@DrakeTDL DrakeTDL force-pushed the stash branch 2 times, most recently from 27ac7fd to 2687ded Compare July 6, 2024 02:00
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 6, 2024
@DrakeTDL DrakeTDL force-pushed the stash branch 2 times, most recently from a7f641e to b9ae77c Compare July 6, 2024 03:30
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes labels Jul 13, 2024
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 13, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 13, 2024
@DrakeTDL
Copy link
Member Author

DrakeTDL commented Sep 8, 2024

Thanks for the review.

@SuperSandro2000
Copy link
Member

Please familiarize yourself with the contributing guide https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md especially on how to name commits. It is appreciated that you try to do tiny commits but probably some of those 24 commits could be combined to make this PR not look as daunting.

@DrakeTDL
Copy link
Member Author

DrakeTDL commented Sep 20, 2024

Please familiarize yourself with the contributing guide https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md especially on how to name commits. It is appreciated that you try to do tiny commits but probably some of those 24 commits could be combined to make this PR not look as daunting.

@SuperSandro2000
Squashed those commits into the first 4.
Should I also squash the update commit into the refactor commit or is it okay as is?

@Retrodynen
Copy link

Anything else needed for this to be merged?

@phanirithvij
Copy link
Member

has newer version 0.27.2

@phanirithvij
Copy link
Member

phanirithvij commented Nov 16, 2024

suggestion: run these in your local checkout

# minor style improvments
echo nixos/tests/stash.nix pkgs/by-name/st/stash/package.nix nixos/modules/services/web-apps/stash.nix | xargs -n1 nix shell nixpkgs#statix.out -c statix check

# this is good (for this pr)
nix shell nixpkgs#deadnix.out -c deadnix nixos/tests/stash.nix pkgs/by-name/st/stash/package.nix nixos/modules/services/web-apps/stash.nix

just a suggestion, also fyi I don't have commiter access.

@DrakeTDL
Copy link
Member Author

Thanks, I'll definitely use them. I believe I fixed most/all of Sandro's suggestions.

@DrakeTDL
Copy link
Member Author

@phanirithvij What would be the next steps to get this merged? Should I re-submit to discourse/matrix "ready for review" and wait?

Copy link
Member

@phanirithvij phanirithvij left a comment

Choose a reason for hiding this comment

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

ran nixosTest locally, LGTM

@phanirithvij
Copy link
Member

@DrakeTDL yes

@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/4843

@phanirithvij

This comment was marked as outdated.

Copy link
Member

@phanirithvij phanirithvij Dec 26, 2024

Choose a reason for hiding this comment

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

any particular reason for this overrideAttrs? can put passthru in buildGoModule itself.

also maybe add passthru.frontend = ui; so people have the ability to override ui build from outside. eg.

and https://github.com/phanirithvij/system/blob/eca1177e783d5a513ee380892e3ac54dbafff63a/nixos/applications/opengist.nix#L74

Copy link
Member Author

Choose a reason for hiding this comment

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

any particular reason for this overrideAttrs? can put passthru in buildGoModule itself.

I initially did that because buildGoModule didn't accept a fixed-point function and I didn't think that I could request itself from the arguments. Don't know why I thought that since it is a package like any other.

As suggested, I inlined passthru and inherited frontend.

DrakeTDL and others added 4 commits January 24, 2025 10:48
Uses git source directly instead of binary releases
Adds an update script
Adds a version test
@aicynide

This comment was marked as off-topic.

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 This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants