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: rewrite (68.2.2 -> 68.3.0) #75328

Merged
merged 2 commits into from Dec 12, 2019

Conversation

@lovesegfault
Copy link
Contributor

@lovesegfault lovesegfault commented Dec 9, 2019

Motivation for this change

This supersedes #75143, adding Wayland support, bumping to 68.3.0, and also reworking the build a bit.

The reason for reworking is I noticed the Firefox and Thunderbird builds had gone astray, and since they are almost identical, we ought to try and keep them as much in sync as possible.

I would appreciate @andir taking a look at this since he maintains Firefox and probably has some insights into it.

I've also added myself as a maintainer of the package.

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 nix-review --run "nix-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.
Notify maintainers

cc @edolstra @nbp

@lovesegfault lovesegfault mentioned this pull request Dec 9, 2019
4 of 10 tasks complete
@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 50ef590 to 6a57758 Dec 9, 2019
@ofborg ofborg bot requested review from edolstra and nbp Dec 9, 2019
@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 6a57758 to 63866c3 Dec 9, 2019
@lovesegfault lovesegfault requested a review from worldofpeace Dec 9, 2019
Copy link
Member

@worldofpeace worldofpeace left a comment

Still building over here.

@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 9, 2019

Before merging I want to take some of the changes of #55026 and integrate them here.

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 29fbb89 to 9144cef Dec 9, 2019
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 9, 2019

Before merging I want to take some of the changes of #55026 and integrate them here.

It'd be cool to also integrate the code style from there, in particular one item per line in arrays.
Not sure about removing the gtk option, but I already prefer having it on default here, maybe we should just remove it.

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 9144cef to b392052 Dec 9, 2019
@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 9, 2019

@worldofpeace Done, I left the arrays as-is because I don't like the 1/line look, if I must I will change it.

I simplified the makeDesktopItem and patchelf stuff, I'd like a suggestion on how to lift the default-toolkit selection off of the script portion, it looks dirty and it's hard to read.

I mean the

    "--enable-default-toolkit=cairo-gtk${if gtk3Support then "3${lib.optionalString waylandSupport "-wayland"}" else "2"}"

line.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 9, 2019

@worldofpeace Done, I left the arrays as-is because I don't like the 1/line look, if I must I will change it.

I simplified the makeDesktopItem and patchelf stuff, I'd like a suggestion on how to lift the default-toolkit selection off of the script portion, it looks dirty and it's hard to read.

I mean the

    "--enable-default-toolkit=cairo-gtk${if gtk3Support then "3${lib.optionalString waylandSupport "-wayland"}" else "2"}"

line.

You could put it in let in

let 
  toolkitSlug = if gtk3Support then ''
    3${optionalString waylandSupport "-wayland"}
  '' else "2";

  toolkitValue = "cairo-gtk${toolkitSlug}"
in

This also reminds me that waylandSupport logically implicates gtk3Support.

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from b392052 to 38feec5 Dec 9, 2019
@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 9, 2019

@worldofpeace Not sure why the eval is borked

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 38feec5 to 601b08d Dec 9, 2019
@vcunat
Copy link
Member

@vcunat vcunat commented Dec 9, 2019

I've been using current state (601b08d) for a couple hours, and it seems fine. (But I haven't really looked at the code, so far.)

Copy link
Member

@andir andir left a comment

I did leave some comments. Mostly about style and due diligence on catching up with our comments in the expression.

I am not a Thunderbird user so I didn't test this.

@vcunat
Copy link
Member

@vcunat vcunat commented Dec 9, 2019

It'd be cool to also integrate the code style from there, in particular one item per line in arrays.

I agree the style does have some advantages, but to me it takes a bit too much space typically and I personally prefer to group them a bit. BTW, that PR did this only for *Inputs and not for the function parameters; I assume you'd want to apply one-item-per-line in there as well, as that's what I see in gnome expressions.

In any case, I don't care much and I want to avoid spending noticeable amount of time on bikeshedding discussions.

@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 9, 2019

How about I just nixfmt it and call it a day?

@andir
Copy link
Member

@andir andir commented Dec 9, 2019

* remove `MOZ_LEGACY_PROFILES`

* remove `MOZ_ALLOW_DOWNGRADE`

(on the advice of upstream maintainers)

I think that will get us into a funny position. At least for Firefox it treats each new binary location as an incompatible install of Firefox and thus refuses to reuse your existing profiles. That is why those are in there. If you recompile the package with a few bits flipped you should be able to reproduce that.

@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 9, 2019

@andir I expected that too, but in my testing so far I have been unable to reproduce that and things work fine. It'd be nice if someone else would confirm no issues, cc. @vcunat

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 10, 2019

I agree the style does have some advantages, but to me it takes a bit too much space typically and I personally prefer to group them a bit. BTW, that PR did this only for *Inputs and not for the function parameters; I assume you'd want to apply one-item-per-line in there as well, as that's what I see in gnome expressions.

Yeah, this happens to be how we've chosen to go about things in gnome expressions.
And using nixpkgs-fmt, I really value diff-ability. Even if the files are long.

@vcunat
Copy link
Member

@vcunat vcunat commented Dec 10, 2019

I believe I did encounter some issues with thunderbird refusing to "downgrade" or something like that, at least when switching testing of -bin and compiled variant (even though my version never decreased). That was just before we merged some of these variables IIRC (several weeks ago?)

I really value diff-ability

Yes, but regardless of this I personally want to use tools that nicely highlight smaller changes on longer lines. (For non-nix code, for example.)

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 634efa4 to a8d50dd Dec 10, 2019
@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 10, 2019

I reverted the changes around legacy profiles for the time being, if they complain during the standardization process we can revisit this.

@worldofpeace, @vcunat, @andir: This is ready for a last look and merge.

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from a8d50dd to 76d07cf Dec 10, 2019
@lovesegfault lovesegfault requested a review from worldofpeace Dec 11, 2019
Copy link
Member

@worldofpeace worldofpeace left a comment

Builds and executes. LGTM.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 11, 2019

@lovesegfault Could you document all the notable changes in the commit msg?

@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 11, 2019

@worldofpeace on it :)

@lovesegfault lovesegfault force-pushed the lovesegfault:thunderbird-rewrite branch from 76d07cf to 262610e Dec 11, 2019
@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 11, 2019

@andir
andir approved these changes Dec 12, 2019
Copy link
Member

@andir andir left a comment

Looks good. No more complains from my side (for now ;)).

@andir
Copy link
Member

@andir andir commented Dec 12, 2019

@worldofpeace once you are happy with functionality/testing/reviews feel free to merge. Nothing against it from my side. Just do not use use it so testing it would be kinda silly...

@worldofpeace worldofpeace force-pushed the lovesegfault:thunderbird-rewrite branch from 262610e to c62e086 Dec 12, 2019
This is a large scale rework of the package, here's a change summary:
    * Organized inputs (1/line, except conditionals)
    * Introduced alsaSupport, pulseaudioSupport, waylandSupport
    * enableGTK3 -> gtk3Support
    * enableCalendar -> calendarSupport
    * Organized buildInputs, nativeBuildInputs (1/line)
    * Corrected native/buildInputs separation
    * Ported over fixes/changes from Firefox
    * Enabled sound, webp, vpx, rust-SIMD, necko-wifi
    * Removed manual wrapping
    * Lifted makeDesktopItem out of string section, into Nix
    * Correctly set bindgen options
    * Added lovesegfault as maintainer
    * New url which uses https
This is non-authoritative, look at the diff for further info.
@worldofpeace worldofpeace force-pushed the lovesegfault:thunderbird-rewrite branch from c62e086 to 8b8458a Dec 12, 2019
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 12, 2019

Pushed silly nitpicks and the new homepage https://www.thunderbird.net/en-US/

@worldofpeace worldofpeace merged commit f7e5482 into NixOS:master Dec 12, 2019
1 check was pending
1 check was pending
grahamcofborg-eval Cloning project
Details
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 12, 2019

Thanks a lot @lovesegfault 🌠
Was nice helping you, thanks for the commits.

vcunat added a commit that referenced this pull request Dec 12, 2019
(cherry picked from commit 3d81015 from #75328)

https://www.thunderbird.net/en-US/thunderbird/68.3.0/releasenotes/
https://www.mozilla.org/en-US/security/advisories/mfsa2019-38/
I've been using also this commit for yet another few hours.
@lovesegfault
Copy link
Contributor Author

@lovesegfault lovesegfault commented Dec 12, 2019

Thanks everyone for helping me push this through :)

@lovesegfault lovesegfault deleted the lovesegfault:thunderbird-rewrite branch Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.