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.

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

drawpile: add build options and split into multiple top-level attrs #57573

Merged
merged 2 commits into from Apr 15, 2019

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Mar 13, 2019

Motivation for this change

Right now the drawpile derivation builds a full package, but someone may want only the server or client. This pr makes it possible and adds some drawpile variants to all-packages.nix.

Is the way I did it ok? Am I unnecessarily polluting the global namespace?

Depends on #57568.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@veprbl
Copy link
Member

veprbl commented Mar 22, 2019

@fgaz What problem are you solving with this? Adding extra variants means burden in maintaining more packages.

@fgaz
Copy link
Member Author

fgaz commented Mar 23, 2019

@veprbl Thanks for the review!

What problem are you solving with this

I want to separate client and server, so people can install for example just the server part without bringing in GUI/client deps, but yeah, I got carried away and added -headless and -kis.

What about:

  • keeping all the options as arguments
  • adding two extra args to enable/disable the optional deps
    • or even an arg for each optional dep (enableGifSupport etc)
  • just adding a -server-headless variant so hydra can build it

@veprbl
Copy link
Member

veprbl commented Mar 23, 2019

adding two extra args to enable/disable the optional deps

I'm not convinced there is such a big value in having knobs to fine tune all these optional dependencies. I think we usually enable all optional stuff unconditionally for maximal convenience. If someone doesn't want to build with giflib they can always use drawpile.override { giflib = null; }. You can mention what dependencies are optional in a comment, this should be more than enough for an advanced user.

just adding a -server-headless variant so hydra can build it

This sounds good. If you expect that most people would rather use either a full gui client and server or a headless server install then it greatly reduces number of options that we need to provide. We can keep -kis variant if you think you need it.

@fgaz
Copy link
Member Author

fgaz commented Apr 10, 2019

Waiting for drawpile/Drawpile#744 for kis. I'll just modify the default without adding any -kis variant if they say to enable it.

@veprbl
Copy link
Member

veprbl commented Apr 11, 2019

It seems like they say that they don't need it. Also you probably don't need to package any special setups unless you use them yourself.

Optional client and server features.

Also add a server-headless variant.
@fgaz
Copy link
Member Author

fgaz commented Apr 12, 2019

All done. Ping @veprbl

@veprbl veprbl merged commit 7ba046d into NixOS:master Apr 15, 2019
@veprbl
Copy link
Member

veprbl commented Apr 15, 2019

@fgaz Thanks!

@fgaz fgaz deleted the drawpile/split branch May 11, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants