821291 - move libmozgnome into libxul

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Having it separate makes things a pain and I can't see any advantage.

Comment 1

[(not currently active) Ted Mielczarek]

We probably use this to make libgnome a soft dependency. I'm not sure that matters anymore.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I expect bug 482156 takes care of that.

Comment 3

[Mike Hommey [:glandium]]

Provided you only make it a static component when MOZ_ENABLE_GCONF and MOZ_ENABLE_GNOMEVFS are both *not* set, it's okay. Note the default is now to both not set, but if people are still building with these enabled, that'd keep it as a separate component for them. That being said, we may want to remove the gconf and gnomevfs code at some point. Not sure if it's too early.

Comment 4

[Landry Breuil (:gaston)]

Wasnt the original intent to separate dependencies at runtime, ie build the whole thing with gnome parts enabled, and package firefox-main and firefox-gnome-integration (or whatever name) separate, only the latter package depending on gnomevfs/gconf and shipping libmozgnome.so ?

Comment 5

[Mike Hommey [:glandium]]

That was the intent, but we don't really need gnomevfs/gconf code now, with gio/gsettings. The problem might be for people backporting to old distros without gio/gsettings, but it might just be a theoretical problem at this point.

Comment 6

[Justin Wood (:Callek)]

fwiw, SeaMonkey still does not build with gio, due to us building on the old Linux refplatform [cent5]

We hope to, before feb 2013, be properly on cent6/mock-based refplatforms though. And thus switched properly to gio

Comment 7

[Karl Tomlinson (:karlt)]

Bug 713827 made the GConf use dynamic, so that is no longer a dependency.
It is too soon to remove GConf code.  Our builders and test machines don't support GSettings, so GSettings is used dynamically where available.  However, if GConf is causing problems like bug 769764 comment 5, then I would consider removing the parts causing problems.

libmozgnome need only be a separate library when MOZ_ENABLE_GNOMEVFS is set.

Comment 8

[Karl Tomlinson (:karlt)]

We no longer have a reason to support gnomevfs (bug 1047752), so mozgnome can now always be part of libxul.so.

Comment 9

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ce9e6af14e

Comment 10

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

Bug 821291 - Move libmozgnome into libxul. r?karlt

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

https://reviewboard.mozilla.org/r/26145/#review23543

Thanks for doing this.  Not granting r+ due to the UaF, but the reset looks good.
Please also get review from glandium on the build file changes.
(All build file changes beyond just changing an existing list require build peer review.)

::: toolkit/system/gnome/moz.build:39
(Diff revision 1)
>  OS_LIBS += CONFIG['MOZ_GCONF_LIBS']
>  OS_LIBS += CONFIG['GLIB_LIBS']
>  OS_LIBS += CONFIG['MOZ_GIO_LIBS']
>  OS_LIBS += CONFIG['MOZ_DBUS_GLIB_LIBS']

Mike would know whether it is appropriate to specify OS_LIBS in every directory, but I would assume that the convention is not to add these libs that are already included libxul.

::: toolkit/system/gnome/nsGIOService.cpp:72
(Diff revision 1)
> -  PromiseFlatCString flatUri(aUri);
> +  uris.data = const_cast<char*>(PromiseFlatCString(aUri).get());
> -  uris.data = const_cast<char*>(flatUri.get());

This change doesn't look necessary and I'm not aware of a reason why the lifetime would necessarily be longer than documented.

https://dxr.mozilla.org/mozilla-central/rev/45273bbed8efaface6f5ec56d984cb9faf4fbb6a/xpcom/string/nsTPromiseFlatString.h#27

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

Mike requesting your review on the build file changes.
Looks good to me, but I don't know what the removal of
@RESPATH@/components/components.manifest means.

Comment 13

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26145/#review23543

> This change doesn't look necessary and I'm not aware of a reason why the lifetime would necessarily be longer than documented.
> 
> https://dxr.mozilla.org/mozilla-central/rev/45273bbed8efaface6f5ec56d984cb9faf4fbb6a/xpcom/string/nsTPromiseFlatString.h#27

I couldn't get it to compile otherwise. :-P The error I saw was "expected ';' before 'flatUri'", followed by "statement cannot resolve address of overloaded function." I see `nsStringAPI.h` just uses a `typedef`, but I guess `nsTPromiseFlatString.h` does something different?

Comment 14

[Lina Butler [:lina]]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26145/diff/1-2/

Comment 15

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/26145/#review23543

> I couldn't get it to compile otherwise. :-P The error I saw was "expected ';' before 'flatUri'", followed by "statement cannot resolve address of overloaded function." I see `nsStringAPI.h` just uses a `typedef`, but I guess `nsTPromiseFlatString.h` does something different?

nsPromiseFlatCString flatUri(aUri);

PromiseFlatCString() is a function when MOZILLA_INTERNAL_API is defined, which is inconsistent with the  typedef in nsStringAPI.h.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

https://reviewboard.mozilla.org/r/26145/#review23549

Comment 17

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26145/#review23543

> nsPromiseFlatCString flatUri(aUri);
> 
> PromiseFlatCString() is a function when MOZILLA_INTERNAL_API is defined, which is inconsistent with the  typedef in nsStringAPI.h.

D'oh. I completely missed the "ns" prefix. Sorry about that!

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26145/diff/2-3/

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

https://reviewboard.mozilla.org/r/26145/#review23553

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

https://reviewboard.mozilla.org/r/26145/#review23555

I /think/ it's good on the build system side of things, but I have some doubt about gio. Please run this through try for *both* Gtk+3 *and* Gtk+2 (a normal push to try will do for the former, and see build/unix/mozconfig.gtk for the latter)

Comment 21

[Mike Hommey [:glandium]]

Relatedly, we should probably remove the --disable-gconf and --disable-gio options too.

Comment 22

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5675951e02

Comment 23

[Lina Butler [:lina]]

With Gtk+2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da329c214f44

Comment 24

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c475c4538070

Comment 25

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8400903fe6e07ccc6f7390c97f0a922837e63c0
Bug 821291 - Move libmozgnome into libxul. r=glandium,karlt

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8400903fe6e