Closed Bug 821291 Opened 7 years ago Closed 4 years ago

move libmozgnome into libxul

Categories

(Toolkit Graveyard :: Build Config, defect)

x86
Linux
defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jrmuizel, Unassigned)

References

Details

Attachments

(1 file)

Having it separate makes things a pain and I can't see any advantage.
We probably use this to make libgnome a soft dependency. I'm not sure that matters anymore.
I expect bug 482156 takes care of that.
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.
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 ?
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.
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
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.
Blocks: 563628
Depends on: 713827
OS: Mac OS X → Linux
We no longer have a reason to support gnomevfs (bug 1047752), so mozgnome can now always be part of libxul.so.
Depends on: 1047752
Blocks: 1227300
Bug 821291 - Move libmozgnome into libxul. r?karlt
Attachment #8691553 - Flags: review?(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
Attachment #8691553 - Flags: review?(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.
Attachment #8691553 - Flags: review?(mh+mozilla)
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?
Attachment #8691553 - Attachment description: MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?karlt → MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt
Attachment #8691553 - Flags: review?(karlt)
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/
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 on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

https://reviewboard.mozilla.org/r/26145/#review23549
Attachment #8691553 - Flags: review?(karlt)
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!
Attachment #8691553 - Flags: review?(karlt)
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 on attachment 8691553 [details]
MozReview Request: Bug 821291 - Move libmozgnome into libxul. r?glandium,karlt

https://reviewboard.mozilla.org/r/26145/#review23553
Attachment #8691553 - Flags: review?(karlt) → review+
Attachment #8691553 - Flags: review?(mh+mozilla) → review+
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)
Relatedly, we should probably remove the --disable-gconf and --disable-gio options too.
https://hg.mozilla.org/mozilla-central/rev/a8400903fe6e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.