Skip to content

redisinsight: init at 2.30.0#252557

Merged
delroth merged 1 commit intoNixOS:masterfrom
gmemstr:redisinsight
Sep 8, 2023
Merged

redisinsight: init at 2.30.0#252557
delroth merged 1 commit intoNixOS:masterfrom
gmemstr:redisinsight

Conversation

@gmemstr
Copy link
Copy Markdown
Member

@gmemstr gmemstr commented Aug 31, 2023

Description of changes

Create the RedisInsight package (#242438).

While Redis provides AppImages and other packages, these are not well versioned, only containing the major version the binary was built for. Thus the only way to get proper versioning, from what I can tell, is to build from source.

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.

Copy link
Copy Markdown
Contributor

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

Please change your commit message(s) to match the commit message conventions of nixpkgs!

@gmemstr gmemstr mentioned this pull request Aug 31, 2023
12 tasks
@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Aug 31, 2023

PR for maintainership added in #252559, once merged I'll remove from this PR to prevent failures.

@gmemstr gmemstr force-pushed the redisinsight branch 2 times, most recently from 7cab113 to fd01e2f Compare August 31, 2023 13:31
@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Aug 31, 2023

Realised it made more sense to place it under pkgs/development/tools/redisinsight instead of pkgs/applications/misc.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages 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. labels Aug 31, 2023
@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Aug 31, 2023

I think I've narrowed it down to the __dirname variable being set incorrectly, and I think a patch will solve the issue but not 100%.

@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Aug 31, 2023

Success! We have a building and running app.

@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Aug 31, 2023

As a sidenote, this could theoretically support all the platforms, but I don't have the ability to test at the moment, so sticking the with default x86 Linux.

@gmemstr gmemstr force-pushed the redisinsight branch 2 times, most recently from 26a3329 to 4d4c9eb Compare September 1, 2023 08:59
@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Sep 6, 2023

@matthiasbeyer Don't mean to bug you but I'd love another review on this :)

@matthiasbeyer
Copy link
Copy Markdown
Contributor

Well I don't think I am qualified to review yarn packaging, sorry.
But one thing I would say is that normally, new packages are not backported (to my knowledge)... besides that I cannot say much, sorry again!

@delroth
Copy link
Copy Markdown
Contributor

delroth commented Sep 8, 2023

But one thing I would say is that normally, new packages are not backported (to my knowledge)... besides that I cannot say much, sorry again!

Per https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-acceptable-for-releases it's perfectly fine to backport new packages. We don't usually do it unless someone cares / requests it, but it's fine to do so.

Copy link
Copy Markdown
Contributor

@delroth delroth left a comment

Choose a reason for hiding this comment

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

Disclaimer: I also don't know much about the yarn ecosystem in nixpkgs. But nothing in this PR seems particularly wrong to me, and in absence of a more qualified reviewer having a look I'd be happy to see this merged.

One tiny comment if you don't mind doing another change to this PR.

@gmemstr gmemstr force-pushed the redisinsight branch 2 times, most recently from 629e796 to ee0b424 Compare September 8, 2023 19:33
@delroth delroth dismissed matthiasbeyer’s stale review September 8, 2023 19:36

The commit message issue was addressed earlier already, assuming this is obsolete based on the latest PR comment.

@delroth
Copy link
Copy Markdown
Contributor

delroth commented Sep 8, 2023

(CI is now failing due to trailing whitespaces.)

@gmemstr
Copy link
Copy Markdown
Member Author

gmemstr commented Sep 8, 2023

Fixed trailing spaces.

@delroth
Copy link
Copy Markdown
Contributor

delroth commented Sep 8, 2023

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

1 package built:
  • redisinsight

@delroth delroth merged commit 57a0c58 into NixOS:master Sep 8, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 8, 2023

Successfully created backport PR for release-23.05:

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: 0 This PR does not cause any packages 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: 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.

3 participants