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

go-task: init at 2.8.1 #93619

Merged
merged 2 commits into from Jul 25, 2020
Merged

go-task: init at 2.8.1 #93619

merged 2 commits into from Jul 25, 2020

Conversation

Parasrah
Copy link
Member

Motivation for this change

to add go-task to nixpkgs (a make alternative). This is my first time contributing to nixpkgs so please let me know if there is anything I'm missing or should have done differently. I didn't try building from source because it appears that go-task uses itself in the build

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.

danieldk
danieldk suggested changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Thanks you for your first contribution 👍 . I have added some comments on how you could improve the derivation.

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/development/tools/go-task/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/go-task/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/go-task/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/go-task/default.nix Outdated Show resolved Hide resolved
danieldk
danieldk suggested changes Jul 25, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Thank you for making all the changes! Now it's almost in a shape to be merged. I added two more suggestions.

Also, the formatting looks a bit unconventional. Not a blocker, but you can format the derivation using standard nixpkgs formatting using the nixpkgs-fmt utility.

pkgs/development/tools/go-task/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/go-task/default.nix Outdated Show resolved Hide resolved
@Parasrah
Copy link
Member Author

I updated the formatting too, was previously looking at godot/default.nix as a reference for the style. I didn't know about nixpkgs-fmt, really happy to discover there is a formatter for nix!

@danieldk
Copy link
Contributor

danieldk commented Jul 25, 2020 •
edited

I only have one small worry before merging this. The go-task binary is task, which is the same name as the binary for taskwarrior. This means that users would not be able to install both packages in their profile side by side. Is this a tool people would typically install in their profile or only in e.g. in the nativeBuildInputs of some other derivation? If the former, we may consider renaming the binary to e.g. go-task.

@ofborg ofborg bot requested a review from kalbasit July 25, 2020 09:26
@Parasrah
Copy link
Member Author

that's a good point, I only started using the tool myself a few weeks ago, but I have it installed via nix-shell for the projects and use it mostly for development. I could definitely see someone having it installed in their profile side by side task warrior. It would probably be best to rename the binary, I will see if I can't get that sorted

@danieldk
Copy link
Contributor

that's a good point, I only started using the tool myself a few weeks ago, but I have it installed via nix-shell for the projects and use it mostly for development. I could definitely see someone having it installed in their profile side by side task warrior. It would probably be best to rename the binary, I will see if I can't get that sorted

Looks great now! I have pushed one more small change (a way of handling the version that is more canonical in nixpkgs). I'll merge the PR when ofborg is happy.

danieldk
danieldk approved these changes Jul 25, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Congrats with your first derivation in nixpkgs 🎉

Result of nixpkgs-review pr 93619 1

1 package built:
- go-task

@danieldk danieldk merged commit 370a71c into NixOS:master Jul 25, 2020
@veprbl veprbl mentioned this pull request Mar 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk approved these changes

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 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
3 participants
@Parasrah @danieldk @Janik-Haag