melody: init at 2.2.1 #97995
Closed
melody: init at 2.2.1 #97995
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the Melody Package (https://github.com/artemanufrij/playmymusic) to the package repository. <!-- Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers. --> - [ ] Tested using sandboxing ([nix.useSandbox](https://nixos.org/nixos/manual/options.html#opt-nix.useSandbox) on NixOS, or option `sandbox` in [`nix.conf`](https://nixos.org/nix/manual/#sec-conf-file) on non-NixOS linux) - Built on platform(s) - [x] NixOS - [ ] macOS - [ ] other Linux distributions - [ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests)) - [x] Tested compilation of all pkgs that depend on this change using `nix-shell -p nixpkgs-review --run "nixpkgs-review wip"` - [x] Tested execution of all binary files (usually in `./result/bin/`) - [ ] Determined the impact on package closure size (by running `nix path-info -S` before and after) - [ ] Ensured that relevant documentation is up to date - [ ] Fits [CONTRIBUTING.md](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md). First, well second, time commiting. Apparently i managed to mess up the former branch. I'm sorry about that.
drewrisinger
suggested changes
Sep 14, 2020
There was a problem hiding this comment.
Hello, and thanks for contributing your first package to nixpkgs!
I have several comments, mostly formatting & style.
- Separate out adding yourself as a maintainer to a separate commit.
git reset --soft HEAD~1, then commit them separately. - Several comments in code body.
My standard review checklist:
- Diff needs some improvement
- Maintainer commit needs separated
- Builds via nix-review:
https://github.com/NixOS/nixpkgs/pull/97995
1 package built:
melody
Comment on lines
+3
to
+20
| , meson | ||
| , ninja | ||
| , gtk3 | ||
| , pkgconfig | ||
| , python3 | ||
| , desktop-file-utils | ||
| , vala | ||
| , wrapGAppsHook | ||
| , appstream | ||
| , clutter-gtk | ||
| , cmake | ||
| , gst_all_1 | ||
| , json-glib | ||
| , libgee | ||
| , libsoup | ||
| , pantheon | ||
| , sqlite | ||
| , taglib |
There was a problem hiding this comment.
Sort these alphabetically
| src = fetchFromGitHub { | ||
| owner = "artemanufrij"; | ||
| repo = "playmymusic"; | ||
| rev = "bba5fe74134ad7533c41044d8ce41f1a8f69b3b0"; |
There was a problem hiding this comment.
Use a tagged release if possible
Suggested change
| rev = "bba5fe74134ad7533c41044d8ce41f1a8f69b3b0"; | |
| rev = "2.2.1"; |
If not using a tagged release, a reason should be given, and the version above should be changed to something like version = "unstable-2020-09-14" (where the date is the date of the commit).
Comment on lines
+35
to
+41
| desktop-file-utils | ||
| meson | ||
| ninja | ||
| pkgconfig | ||
| python3 | ||
| vala | ||
| wrapGAppsHook |
Comment on lines
+44
to
+61
| buildInputs = with gst_all_1; [ | ||
| appstream | ||
| clutter-gtk | ||
| cmake | ||
| gst-plugins-bad | ||
| gst-plugins-base | ||
| gst-plugins-good | ||
| gst-plugins-ugly | ||
| gstreamer | ||
| gtk3 | ||
| json-glib | ||
| libgee | ||
| libsoup | ||
| pantheon.elementary-icon-theme | ||
| pantheon.granite | ||
| sqlite | ||
| taglib | ||
| ]; |
There was a problem hiding this comment.
To clarify which are from gst_all_1, I recommend splitting this into two lists:
buildInputs = [
non-gst_all_1_pkgs
] ++
(with gst_all_1; [ gst_all_1_pkgs ]);
|
|
||
| postPatch = '' | ||
| chmod +x meson/post_install.py | ||
| patchShebangs meson/post_install.py |
There was a problem hiding this comment.
Is this script automatically run?
| meta = with stdenv.lib; { | ||
| description = "A music player for listening to local music files, online radio and Audio CD's - Designed for elementary OS"; | ||
| homepage = "https://anufrij.org/melody"; | ||
| license = licenses.gpl3; |
There was a problem hiding this comment.
Suggested change
| license = licenses.gpl3; | |
| license = licenses.gpl3Plus; |
Using gpl3 is deprecated. The source seems to hint that GPL3 or later are acceptable licenses.
Comment on lines
+27971
to
+27972
| melody = callPackage ../applications/audio/melody {}; | ||
|
|
There was a problem hiding this comment.
This typically wouldn't get put at the end, but would probably be sorted into semi-alphabetical order in the range of lines 20000+ (don't have the file in front of me, so rough range)
Add myself to maintainers-list.nix - [ ] Tested using sandboxing ([nix.useSandbox](https://nixos.org/nixos/manual/options.html#opt-nix.useSandbox) on NixOS, or option `sandbox` in [`nix.conf`](https://nixos.org/nix/manual/#sec-conf-file) on non-NixOS linux) - Built on platform(s) - [ ] NixOS - [ ] macOS - [ ] other Linux distributions - [ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests)) - [ ] Tested compilation of all pkgs that depend on this change using `nix-shell -p nixpkgs-review --run "nixpkgs-review wip"` - [ ] Tested execution of all binary files (usually in `./result/bin/`) - [ ] Determined the impact on package closure size (by running `nix path-info -S` before and after) - [ ] Ensured that relevant documentation is up to date - [ ] Fits [CONTRIBUTING.md](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md).
add myself to maintainers-list.nix <!-- Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers. --> - [ ] Tested using sandboxing ([nix.useSandbox](https://nixos.org/nixos/manual/options.html#opt-nix.useSandbox) on NixOS, or option `sandbox` in [`nix.conf`](https://nixos.org/nix/manual/#sec-conf-file) on non-NixOS linux) - Built on platform(s) - [ ] NixOS - [ ] macOS - [ ] other Linux distributions - [ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside [nixos/tests](https://github.com/NixOS/nixpkgs/blob/master/nixos/tests)) - [ ] Tested compilation of all pkgs that depend on this change using `nix-shell -p nixpkgs-review --run "nixpkgs-review wip"` - [ ] Tested execution of all binary files (usually in `./result/bin/`) - [ ] Determined the impact on package closure size (by running `nix path-info -S` before and after) - [ ] Ensured that relevant documentation is up to date - [ ] Fits [CONTRIBUTING.md](https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md).
|
Apparently, i'm able to mess up things but not to fix em. Closing this and might pick up up later on... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
Add the Melody Package (https://github.com/artemanufrij/playmymusic) to the package repository.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)First, well second, time commiting. Apparently, i managed to mess up the former branch. I'm sorry about that.