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

somafm-cli: init at 0.3.1 #96388

Merged
merged 1 commit into from Aug 29, 2020
Merged

somafm-cli: init at 0.3.1 #96388

merged 1 commit into from Aug 29, 2020

Conversation

SuperSandro2000
Copy link
Member

Motivation for this change

somafm-cli wasn't already in the package repo.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf 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)
  • 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.

stigtsp
stigtsp requested changes Aug 27, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp left a comment •
edited

Choose a reason for hiding this comment

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

Builds fine on x86_64-linux

Result of nixpkgs-review pr 96388 1

1 package built:
- somafm-cli

Tested executable OK.

pkgs/tools/misc/somafm-cli/default.nix Outdated Show resolved Hide resolved
stigtsp
stigtsp requested changes Aug 29, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Hi!

make install doesn't work on Darwin, and the Makefile seems redundant too since it's essentially a single shell script.

I suggest installing src/somafm directly into $out/bin/somafm.

Also can you make this PR one commit?

pkgs/tools/misc/somafm-cli/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/somafm-cli/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/somafm-cli/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member Author

SuperSandro2000 commented Aug 29, 2020 •
edited

make install doesn't work on Darwin, and the Makefile seems redundant too since it's essentially a single shell script.

Yeah, you are right. I was a bit lazy when packaging.

Also can you make this PR one commit?

Done and force pushed.

Thanks for the great feedback.

I also did another test with nix-shell -A somafm-cli --pure and with network and it still works great.

stigtsp
stigtsp requested changes Aug 29, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp left a comment •
edited

Choose a reason for hiding this comment

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

Just a nitpick: stdenv.lib.makeBinPath will make building the PATH a bit easier

pkgs/tools/misc/somafm-cli/default.nix Outdated Show resolved Hide resolved
stigtsp
stigtsp approved these changes Aug 29, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp 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

Thanks for the contribution, and welcome to the NixOS project.

@stigtsp stigtsp requested a review from danieldk August 29, 2020 17:41
@SuperSandro2000
Copy link
Member Author

I just did a rebase with master to remove the commit which added me as a maintainer since it is no longer needed after #96576.

@stigtsp stigtsp requested a review from aanderse August 29, 2020 18:40
@ehmry ehmry merged commit 7b0cebf into NixOS:master Aug 29, 2020
@SuperSandro2000 SuperSandro2000 deleted the somafm branch August 29, 2020 22:58
@SuperSandro2000 SuperSandro2000 mentioned this pull request Aug 29, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stigtsp stigtsp stigtsp approved these changes

@danieldk danieldk Awaiting requested review from danieldk

@aanderse aanderse Awaiting requested review from aanderse

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@SuperSandro2000 @danieldk @stigtsp @aanderse @ehmry @Janik-Haag