Skip to content

Comments

stown: new package contribution#389975

Merged
GaetanLepage merged 2 commits intoNixOS:masterfrom
rseichter:add-maintainer
Mar 22, 2025
Merged

stown: new package contribution#389975
GaetanLepage merged 2 commits intoNixOS:masterfrom
rseichter:add-maintainer

Conversation

@rseichter
Copy link
Contributor

@rseichter rseichter commented Mar 15, 2025

New package "stown", new maintainer entry.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Mar 15, 2025
@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Mar 15, 2025
@rseichter rseichter changed the title maintainer-list.nix: add rseichter stown: new package contribution Mar 15, 2025
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Welcome to Nixpkgs! Please review the commit conventions for maintainers/; your maintainer commit should be named maintainers: add rseichter.

@SigmaSquadron SigmaSquadron added the 11.by: upstream-developer This issue or PR was created by the developer of packaged software. label Mar 15, 2025
In preparation for an upcoming new package contribution.
@rseichter
Copy link
Contributor Author

@SigmaSquadron Thank you for the review, that was mighty fast. I hope I addressed the points you mentioned in the proper way. My original package.nix was mostly a copy of an existing Python package build, and I opted to make as few changes as possible, seeing how this is my first ever Nix package contribution. Please let me know if anything else needs fixing.

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 15, 2025
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Looks good to me! The binary seems to build and run just fine.


nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 389975

x86_64-linux

✅ 2 packages built:
  • stown
  • stown.dist

@SigmaSquadron SigmaSquadron added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 15, 2025
@rseichter
Copy link
Contributor Author

@SigmaSquadron Just to make sure that I understand the process correctly: At this point I don't need to take any further action on my end, unless specifically told to do so, correct? In other words, I just wait for this pull request to be merged eventually?

@SigmaSquadron
Copy link
Contributor

That's right. Soon enough a committer will find this PR, conduct a final review, and merge it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python3Packages.unittestCheckHook
python3Packages.pytestCheckHook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The upstream tests are written and intended for unittest exclusively. While they should theoretically work with pytest, that is neither guaranteed nor verified, so I don't feel comfortable introducing a dependency for the NixOS package alone.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong, but some maintainer once told me that pytestCheckHook is preferred when possible. No change has to be done upstream.

Wdyt @SuperSandro2000?

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked confirmation on the python matrix channel, and hexa confirmed that pytest should be preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I switched to pytest as you requested, with the caveat that if tests should fail at some point in the future, that constitutes what Douglas Adams classifies as Somebody Else's Problem. Others may prefer pytest, but I am planning to stick with Python's internal unittest framework for this package's upstream development. This is not meant to cause a fuzz, just for clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! By no means nixpkgs packaging should enforce this kind of decision in the upstream repo.
The decision here was only to run your tests with pytest rather than with unittest.
If for some reason it would not have been possible, we would have fallback to unittest. You are rightfully free to do whatever you want upstream :)

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 389975


x86_64-linux

✅ 2 packages built:
  • stown
  • stown.dist

aarch64-linux

✅ 2 packages built:
  • stown
  • stown.dist

x86_64-darwin

✅ 2 packages built:
  • stown
  • stown.dist

aarch64-darwin

✅ 2 packages built:
  • stown
  • stown.dist

Manage file system object mapping via symlinks. Lightweight
alternative to GNU Stow.

Signed-off-by: Ralph Seichter <github@seichter.de>
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 389975


x86_64-linux

✅ 2 packages built:
  • stown
  • stown.dist

aarch64-linux

✅ 2 packages built:
  • stown
  • stown.dist

x86_64-darwin

✅ 2 packages built:
  • stown
  • stown.dist

aarch64-darwin

✅ 2 packages built:
  • stown
  • stown.dist

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Thanks @rseichter !

@GaetanLepage GaetanLepage merged commit cb3270f into NixOS:master Mar 22, 2025
25 of 29 checks passed
@rseichter
Copy link
Contributor Author

Thank you @GaetanLepage and @SigmaSquadron for your time helping me with this PR. Have a nice weekend.

@rseichter rseichter deleted the add-maintainer branch March 22, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 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. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: upstream-developer This issue or PR was created by the developer of packaged software. 12.approvals: 1 This PR was reviewed and approved by one person. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants