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

thunderbird: add option to enable calendar #24851

Merged
merged 1 commit into from Apr 12, 2017

Conversation

laMudri
Copy link
Contributor

@laMudri laMudri commented Apr 12, 2017

Motivation for this change

The Lightning addon for Thunderbird appears to have been deprecated in favour of the built-in version of the same thing. This change allows the thunderbird package to be built with calendar support via the option thunderbird.enableCalendar.

I currently have it disabled by default. Possibly it should be enabled by default, given that official builds come with the calendar. I don't know whether there are any best practices for this sort of thing.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@laMudri, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @vcunat and @taku0 to be potential reviewers.

Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

I agree that it should be enabled by default if official builds have it.

@@ -7,7 +7,7 @@
, autoconf213, which, m4
, writeScript, xidel, common-updater-scripts, coreutils, gnused, gnugrep, curl
, enableGTK3 ? false, gtk3, wrapGAppsHook
, debugBuild ? false
, debugBuild ? false, config
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use config in a derivation, instead make a new flag argument (enableCalendar or something like that). You can set this flag from config in all-packages.nix. Personally I think that config shouldn't be used for single-package options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, sure. Should I do a fixup commit, rebase -i, and force push? Or just add more commits?

Copy link
Member

Choose a reason for hiding this comment

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

Force-push is usually okay (and we'll need to squash it in the end anyway). Thanks!

@edolstra
Copy link
Member

Yeah enabling it by default sounds fine. And adding a config option may be overkill.

@laMudri
Copy link
Contributor Author

laMudri commented Apr 12, 2017

I changed it to be enabled by default, with no config option.

@edolstra edolstra merged commit 75f1a55 into NixOS:master Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants