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.

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

numbat: init at 1.6.3 #264241

Merged
merged 3 commits into from
Nov 2, 2023
Merged

numbat: init at 1.6.3 #264241

merged 3 commits into from
Nov 2, 2023

Conversation

giomf
Copy link
Contributor

@giomf giomf commented Oct 29, 2023

Description of changes

Hi,

I want to contribute my first package called numbat which is a High precision scientific calculator with full support for physical units.
Since it is my first package please point me into the right direction if I did something wrong.

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/)
  • 23.11 Release Notes (or backporting 23.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.

@h7x4
Copy link
Member

h7x4 commented Oct 29, 2023

Hello @giomf, welcome to nixpkgs!

We have a few contributing guidelines as you might've seen, in particular I'd like to point you to the commit conventions. You'll have to split up your commit into two - one that adds you to the maintainer list and another which is the init numbat commit.

@giomf
Copy link
Contributor Author

giomf commented Oct 29, 2023

Yes I already saw this but I have a question regarding the maintainer commit:
How to prefix it since its not related to any package?
From the docs: (pkg-name | nixos/<module>): (from -> to | init at version | refactor | etc)

Edit: nvm I just saw the documentation piece that I need...

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Oct 29, 2023
Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

Not much to comment on here, neat!

pkgs/by-name/nu/numbat/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/nu/numbat/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/nu/numbat/package.nix Outdated Show resolved Hide resolved
@natsukium
Copy link
Member

@ofborg eval

Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

Built and tested application, works nicely!

githubId checks out: https://api.github.com/user/35076723

  • package path fits guidelines
  • package name fits guidelines
  • package version fits guidelines
  • package build on x86_64-linux
  • executables tested on x86_64-linux
  • meta.description is set and fits guidelines
  • meta.license fits upstream license
  • meta.platforms is set
  • meta.maintainers is set
  • build time only dependencies are declared in nativeBuildInputs
  • source is fetched using the appropriate function
  • the list of phases is not overridden
  • when a phase (like installPhase) is overridden it starts with runHook preInstall and ends with runHook postInstall.
  • patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed
  • patches that are remotely available are fetched rather than vendored

Result of nixpkgs-review pr 264241 run on x86_64-linux 1

1 package built:
  • numbat

Marking as approved now, with the assumption that meta.changelog gets fixed

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 30, 2023
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 30, 2023
@giomf
Copy link
Contributor Author

giomf commented Oct 31, 2023

@h7x4 have I missed something or why does it takes so long to complete the checks?

@h7x4
Copy link
Member

h7x4 commented Oct 31, 2023

It's just the aarch64-darwin (read: ARM-based Apple mac, M1/M2 chip) builder left. We don't really have a lot of machines which can run these tests, so they usually take a really long time (if they even complete at all). The fact that the tests run fine on x86_64 darwin, x86_64 linux and aarch64-linux is a good sign, and since it's not a very platform specific package, it will probably work on aarch64-darwin

@giomf
Copy link
Contributor Author

giomf commented Oct 31, 2023

Ok, thanks for the clarification.
So I will have to wait before I can merge...

@h7x4
Copy link
Member

h7x4 commented Oct 31, 2023

I don't think so. Not necessarily at least. You could ask someone with an ARM mac to verify that the package works correctly if you suspect it won't, but I think this is ready for merge despite the queued test. You'll just have to wait for someone with commit rights to have a look at it.

I'm not sure how familiar you are with the process of getting things merged in nixpkgs, considering this is your first PR. It can be a bit of a slow process, because we have so many PRs compared to the number of reviewers and committers. There are ways you can try to ask for more reviews, namely through the "PRs ready for *" threads on discourse, or in our matrix dev channels.

Alternatively, if you feel like you're up to the challenge, we would be really happy to have some more reviewers. There are several ways to help out with this that doesn't require deep internal knowledge of nix, NixOS or nixpkgs. Something like just building and testing out the software (using tools like nixpkgs-review) in the different PRs would be a great help. This would speed up the process for everyone.

@h7x4 h7x4 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 31, 2023
@giomf
Copy link
Contributor Author

giomf commented Nov 1, 2023

I'm not sure how familiar you are with the process of getting things merged in nixpkgs, considering this is your first PR. It can be a bit of a slow process, because we have so many PRs compared to the number of reviewers and committers. There are ways you can try to ask for more reviews, namely through the "PRs ready for *" threads on discourse, or in our matrix dev channels.

Thanks for the hint. I will open a thread.

Alternatively, if you feel like you're up to the challenge, we would be really happy to have some more reviewers. There are several ways to help out with this that doesn't require deep internal knowledge of nix, NixOS or nixpkgs. Something like just building and testing out the software (using tools like nixpkgs-review) in the different PRs would be a great help. This would speed up the process for everyone.

This sounds good. I would love to help where I can.

@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-already-reviewed/2617/1191

@natsukium
Copy link
Member

Result of nixpkgs-review pr 264241 at d074b63 run on aarch64-darwin 1

1 package failed to build:

@natsukium
Copy link
Member

Whenever you need a review or merge, please contact me.
I may not always be able to do it, but I'm sure I can help.

Co-authored-by: OTABI Tomoya <tomoya.otabi@gmail.com>
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 1, 2023
@natsukium
Copy link
Member

Result of nixpkgs-review pr 264241 at 53da5fe run on aarch64-darwin 1

1 package built successfully:
  • numbat

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 1, 2023
Copy link
Member

@natsukium natsukium left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@natsukium natsukium merged commit dd4cf78 into NixOS:master Nov 2, 2023
@giomf giomf deleted the numbat-init branch November 2, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable 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.

6 participants