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

audiality2, koboredux: init #85203

Merged
merged 2 commits into from Sep 23, 2020
Merged

audiality2, koboredux: init #85203

merged 2 commits into from Sep 23, 2020

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Apr 14, 2020

Motivation for this change

Great game similar to the already packaged kobodeluxe. I think packaging also the proprietary part is fair, since in doing so we encourage a sensible foss business model. If you are a nixpkgs committer and want to test the proprietary assets, you can contact me and I'll send you a key.

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.

@fgaz fgaz changed the title audiality2 and koboredux: init audiality2, koboredux: init Apr 14, 2020
@fgaz fgaz force-pushed the koboredux/init branch 2 times, most recently from 282f0b5 to 7541e0a Compare April 14, 2020 10:28
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/160

timokau
timokau approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Very neat! Everything looks good to me. I think I can trust you that you have tested the proprietary assets, though the offer is appreciated :)

One thing to consider: We could also default useProprietaryAssets to true. This would

  • make sure the users are getting the experience the developer intended by default
  • further encourage the business model, which indeed looks very fair

We could then add another attribute, koboredux-free, that disables the proprietary assets. It could also override meta.description appropriately to show the relation. The error message of requireFile could point people to the free version.

The biggest downside is that the unfree license could prevent people from even giving it a shot and seeing the error message. Those people would probably find the -free version the same way they would have found the unfree version though.

What do you think? I don't feel very strongly about this either way. If you prefer it the way it is now, I'll merge.

@fgaz
Copy link
Member Author

fgaz commented May 16, 2020

@timokau that's a great idea, I'll change it.

The biggest downside is that the unfree license could prevent people from even giving it a shot and seeing the error message

Yes, this is a shame, I wish we had an exception or a special license attribute for this case. If the descriptions reference each other it isn't so bad though

@timokau
Copy link
Member

timokau commented May 18, 2020

Yeah, I doubt many people would be bothered by unfree assets in practice (especially if there are free substitutes).

Great, ping me when this is ready for another review.

@fgaz
Copy link
Member Author

fgaz commented Jun 18, 2020

(busy times, I'll pick this up again sometimes this month)

/marvin opt-in
/status needs_work

@marvin-mk2
Copy link

marvin-mk2 bot commented Jun 18, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. The stages are

  • needs_review, if the author considers this PR ready
  • needs_work if the PR in its current form is not ready yet. Maybe the reviewer requested changes, there is an ongoing discussion or you are waiting for upstream feedback.
  • needs_merge can be set by reviewers who do not have merge permission but would merge this PR if they could.

Anybody can switch the current status with a comment of the form /status <new_status_here>.

Feedback and contributions to this bot are appreciated.

@fgaz
Copy link
Member Author

fgaz commented Aug 8, 2020

Well, that was a long time for a couple of string changes 😅

@timokau how about this? too pushy?

@marvin-mk2 marvin-mk2 bot requested a review from timokau August 13, 2020 01:03
@marvin-mk2 marvin-mk2 bot requested a review from ryantm August 24, 2020 10:30
@marvin-mk2
Copy link

marvin-mk2 bot commented Aug 29, 2020

@glittershark please review.

@fgaz
Copy link
Member Author

fgaz commented Aug 30, 2020

/status awaiting_changes

glittershark
glittershark approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@glittershark glittershark left a comment

Choose a reason for hiding this comment

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

Code LGTM besides the existing comment about the too-long description. Happy to test if necessary, but it seems like that's already been taken care of?

timokau
timokau requested changes Sep 23, 2020
View reviewed changes
Copy link
Member

@timokau timokau 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 otherwise.

pkgs/games/koboredux/default.nix Show resolved Hide resolved
@timokau timokau mentioned this pull request Sep 23, 2020
Merged
10 tasks
@timokau
Copy link
Member

timokau commented Sep 23, 2020

Thanks!

@timokau timokau merged commit 2f35207 into NixOS:master Sep 23, 2020
@fgaz fgaz deleted the koboredux/init branch September 23, 2020 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glittershark glittershark glittershark approved these changes

@timokau timokau timokau approved these changes

@ryantm ryantm Awaiting requested review from ryantm

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@fgaz @nixos-discourse @timokau @glittershark