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.

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

node-glob: use buildNpmPackage #250077

Merged
merged 1 commit into from
Aug 31, 2023
Merged

node-glob: use buildNpmPackage #250077

merged 1 commit into from
Aug 31, 2023

Conversation

dotlambda
Copy link
Member

Description of changes

See #229475

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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.

@dotlambda dotlambda added the 9.needs: maintainer Package or module needs active maintainers label Aug 19, 2023
@dotlambda dotlambda requested a review from fgaz August 19, 2023 15:54
fgaz
fgaz approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@fgaz fgaz 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 but needs a rebase. I left a bunch of suggestions/nitpicks

3 packages built:
glob openmoji-black openmoji-color

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

3 packages built:
  • glob
  • openmoji-black
  • openmoji-color

pkgs/tools/misc/glob/default.nix Outdated
}:

buildNpmPackage rec {
pname = "glob";
Copy link
Member

Choose a reason for hiding this comment

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

we might want to name it node-glob like most distros

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the pname should definitely be glob. But we can call the attribute node-glob.

pkgs/tools/misc/glob/default.nix Outdated
description = "The most correct and second fastest glob implementation in JavaScript";
homepage = "https://github.com/isaacs/node-glob";
license = lib.licenses.isc;
mainProgram = "glob";
Copy link
Member

Choose a reason for hiding this comment

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

if we name the package "glob", this line is unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

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

not anymore: #246386

Copy link
Member

Choose a reason for hiding this comment

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

Wow, looks like I have a few patches to submit then

pkgs/tools/misc/glob/default.nix Outdated

meta = {
changelog = "https://github.com/isaacs/node-glob/blob/${src.rev}/changelog.md";
description = "The most correct and second fastest glob implementation in JavaScript";
Copy link
Member

@fgaz fgaz Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This reads more like a slogan than a description. Maybe simply "Glob implementation in JavaScript"

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with the most common description on Repology.

@ofborg ofborg bot requested a review from fgaz August 29, 2023 16:56
Lord-Valen
Lord-Valen reviewed Aug 29, 2023
View reviewed changes
Copy link
Contributor

@Lord-Valen Lord-Valen left a comment

Choose a reason for hiding this comment

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

Just needs a maintainer.

@dotlambda dotlambda changed the title glob: use buildNpmPackage node-glob: use buildNpmPackage Aug 29, 2023
@natsukium natsukium mentioned this pull request Aug 31, 2023
Open
@fgaz
Copy link
Member

fgaz commented Aug 31, 2023

@dotlambda would you like to maintain the package?

@dotlambda
Copy link
Member Author

@dotlambda would you like to maintain the package?

No.

fgaz
fgaz approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Haha well I tried. Let's merge then, it's not like this had a maintainer before, and node-packages.nix is prone to conflicts

@fgaz fgaz merged commit a557591 into NixOS:master Aug 31, 2023
22 checks passed
@dotlambda dotlambda deleted the glob-buildNpmPackage branch August 31, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lord-Valen Lord-Valen Lord-Valen left review comments

@fgaz fgaz fgaz approved these changes

Assignees
No one assigned
Labels
6.topic: nodejs 8.has: package (new) 9.needs: maintainer Package or module needs active maintainers 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dotlambda @fgaz @Lord-Valen