Last Comment Bug 716397 - [Linux] "Warning: package error or possible missing or unnecessary file: bin/libmozglue.so (package-manifest, 34)."
: [Linux] "Warning: package error or possible missing or unnecessary file: bin/...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All Linux
: P1 normal (vote)
: Firefox 12
Assigned To: Jon Buckley
:
Mentors:
Depends on: 701371
Blocks: 713132 713133 716395 770628
  Show dependency treegraph
 
Reported: 2012-01-08 11:19 PST by Serge Gautherie (:sgautherie)
Modified: 2012-07-03 15:10 PDT (History)
3 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix mozglue on Linux packaging warning (884 bytes, patch)
2012-01-13 20:53 PST, Jon Buckley
bugzillamozillaorg_serge_20140323: feedback-
Details | Diff | Splinter Review
v2 Fix mozglue on Linux packaging warning (1.62 KB, patch)
2012-01-14 16:50 PST, Jon Buckley
no flags Details | Diff | Splinter Review
v3 Fix mozglue on Linux packaging warning (1.58 KB, patch)
2012-01-14 20:10 PST, Jon Buckley
khuey: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
v4 Fix mozglue on Linux packaging warning [Checked in: Comment 21] (2.27 KB, patch)
2012-01-19 11:30 PST, Jon Buckley
jon: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-01-08 11:19:09 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=8408639&tree=Firefox&full=1
Linux mozilla-central build on 2012-01-08 06:05:34 PST for push 9a230265bad5

{
Warning: package error or possible missing or unnecessary file: bin/libmozutils.so (package-manifest, 34).
}
Comment 1 Bill Gianopoulos [:WG9s] 2012-01-08 11:23:29 PST
I see this on my builds as well.
Comment 2 Jon Buckley 2012-01-08 12:56:31 PST
I think this might be my fault, I added $(DIST)bin/libmozutils$(DLL_SUFFIX) to the js shell zip in bug 691876 https://bug691876.bugzilla.mozilla.org/attachment.cgi?id=577705

Looking further down at the log:
Packaging JavaScript Shell...
rm -f ../../dist/jsshell-linux-i686.zip
/usr/bin/zip -9j ../../dist/jsshell-linux-i686.zip ../../dist/bin/js ../../dist/bin/mozutils.so  ../../dist/bin/libnspr4.so ../../dist/bin/libplds4.so ../../dist/bin/libplc4.so 
	zip warning: name not matched: ../../dist/bin/mozutils.so
  adding: js (deflated 59%)
  adding: libnspr4.so (deflated 58%)
  adding: libplds4.so (deflated 56%)
  adding: libplc4.so (deflated 59%)

Since jsshell works fine on Linux without requiring mozutils I should probably add a platform #ifdef here...
Comment 3 Serge Gautherie (:sgautherie) 2012-01-11 09:22:50 PST
Ftr, bug 701371 renamed the file:
{
Warning: package error or possible missing or unnecessary file: bin/libmozglue.so (package-manifest, 34).
}
Comment 4 Jon Buckley 2012-01-13 20:53:57 PST
Created attachment 588592 [details] [diff] [review]
Fix mozglue on Linux packaging warning

mozglue is built as a static library on Linux, so it can't be packaged.

I filed bug 718151 to fix the js shell packaging errors.
Comment 5 Serge Gautherie (:sgautherie) 2012-01-14 08:40:14 PST
Comment on attachment 588592 [details] [diff] [review]
Fix mozglue on Linux packaging warning

Review of attachment 588592 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jon Buckley [:jbuck] from comment #4)
> mozglue is built as a static library on Linux, so it can't be packaged.

Right:
http://mxr.mozilla.org/mozilla-central/source/mozglue/build/Makefile.in#57
{
58 # Build mozglue as a shared lib on Windows, OSX and Android.
59 ifneq (,$(filter WINNT Darwin Android,$(OS_TARGET)))
60 FORCE_SHARED_LIB = 1
61 else
62 FORCE_STATIC_LIB = 1
63 endif
}

::: browser/installer/package-manifest.in
@@ +43,5 @@
>  @BINPATH@/@DLL_PREFIX@plds4@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@xpcom@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@nspr4@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@mozalloc@DLL_SUFFIX@
> +#ifndef XP_UNIX

You should match Makefile conditions:
#ifdef "Android"(!?)
#elifdef XP_MACOSX
#elifdef XP_WIN32
Or update Makefile if "Not Unix" is what is actually wanted.
Otherwise OS/2 won't like it, for example.

PS: Does (WINNT+)XP_WIN32 subsumes x86_64?
Comment 6 Jon Buckley 2012-01-14 16:50:01 PST
Created attachment 588696 [details] [diff] [review]
v2 Fix mozglue on Linux packaging warning

Change #ifdefs to match mozglue Makefile, and add comments noting that they need to be kept up to date with each other.
Comment 7 Serge Gautherie (:sgautherie) 2012-01-14 18:43:10 PST
Comment on attachment 588696 [details] [diff] [review]
v2 Fix mozglue on Linux packaging warning

Review of attachment 588696 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, yes, that's the idea :-)

::: browser/installer/package-manifest.in
@@ +43,5 @@
>  @BINPATH@/@DLL_PREFIX@plds4@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@xpcom@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@nspr4@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@mozalloc@DLL_SUFFIX@
> +; This must be kept up to date with /mozglue/build/Makefile.in

I think this comment is implicit: a rule of package-manifest.in.

@@ +44,5 @@
>  @BINPATH@/@DLL_PREFIX@xpcom@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@nspr4@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@mozalloc@DLL_SUFFIX@
> +; This must be kept up to date with /mozglue/build/Makefile.in
> +#if defined(XP_WIN) || defined(XP_MACOSX) || defined(ANDROID)

I prefer alphabetical order, but that may just be a nit of mine.
defined() and || syntax is not (currently) used in package-manifest.in, but, if it works, I support it.

::: mozglue/build/Makefile.in
@@ +55,5 @@
>  CPPSRCS = dummy.cpp
>  endif
>   
>  # Build mozglue as a shared lib on Windows, OSX and Android.
> +# This must be kept up to date with */installer/package-manifest.in

I think Makefile.in shouldn't care about package-manifest.in.
Comment 8 Serge Gautherie (:sgautherie) 2012-01-14 18:47:45 PST
Comment on attachment 588696 [details] [diff] [review]
v2 Fix mozglue on Linux packaging warning

Review of attachment 588696 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/package-manifest.in
@@ +44,5 @@
>  @BINPATH@/@DLL_PREFIX@xpcom@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@nspr4@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@mozalloc@DLL_SUFFIX@
> +; This must be kept up to date with /mozglue/build/Makefile.in
> +#if defined(XP_WIN) || defined(XP_MACOSX) || defined(ANDROID)

Wrt my previous question,
only XP_WIN32 is currently used in package-manifest.in.
I don't know whether XP_WIN is supported/better/....
Comment 9 Jon Buckley 2012-01-14 18:51:03 PST
> defined() and || syntax is not (currently) used in package-manifest.in, but,
> if it works, I support it.

It doesn't work, that syntax isn't supported in config/Expression.py. I'm going to see how difficult it is for me to add support.

> Wrt my previous question,
> only XP_WIN32 is currently used in package-manifest.in.
> I don't know whether XP_WIN is supported/better/....

I assumed that it would cover 32/64, but I'm not sure without actually getting a working build on try server
Comment 10 Serge Gautherie (:sgautherie) 2012-01-14 18:56:52 PST
"Mid-air collision detected!": exactly what I was writing ;-)

(In reply to Jon Buckley [:jbuck] from comment #9)

> It doesn't work, that syntax isn't supported in config/Expression.py. I'm
> going to see how difficult it is for me to add support.

Yeah :-) File a separate bug if it looks good: don't block this one, then update all uses at once.

> I assumed that it would cover 32/64, but I'm not sure without actually

I never cared to check myself: ftb, I assume XP_WIN32 is a legacy/misleading name.

> getting a working build on try server

Probably the best confirmation, if needed.
Comment 11 Jon Buckley 2012-01-14 20:10:39 PST
Created attachment 588706 [details] [diff] [review]
v3 Fix mozglue on Linux packaging warning

Callek had a much easier solution for this problem on IRC:

> Callek: jbuck: my opinion, stick in browser/installer/Makefile.in and browser/locales/Makefile.in a new DEFINES+=MOZGLUE_IS_STATIC=1 and do ifndef MOZGLUE_IS_STATIC (or whatever) in the manifest ;-)

I didn't need to change browser/locales/Makefile.in, but it built successfully, so hooray? Currently testing on the try server now: https://tbpl.mozilla.org/?tree=Try&rev=2ccbccaf0d7d
Comment 12 Serge Gautherie (:sgautherie) 2012-01-14 20:46:29 PST
Comment on attachment 588706 [details] [diff] [review]
v3 Fix mozglue on Linux packaging warning

Review of attachment 588706 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/Makefile.in
@@ +93,5 @@
>  endif
>  
> +ifneq (,$(filter WINNT Darwin Android,$(OS_TARGET)))
> +DEFINES += -DSHARED_MOZGLUE=1
> +endif

This one is obviously fine :-)

Nit: perfect would be if there was an existing variable to reuse, as in
{
ifdef MOZ_ENABLE_GNOME_COMPONENT
DEFINES += -DMOZ_ENABLE_GNOME_COMPONENT=1
endif
}
but is there?
Comment 13 Serge Gautherie (:sgautherie) 2012-01-14 20:55:01 PST
(In reply to Jon Buckley [:jbuck] from comment #11)

> Callek had a much easier solution for this problem on IRC:

Thanks to him ;-)

> I didn't need to change browser/locales/Makefile.in

Indeed, I don't think anything specific to 'locales' is involved wrt 'mozglue', is it?
Comment 14 Justin Wood (:Callek) 2012-01-14 20:56:09 PST
(In reply to Jon Buckley [:jbuck] from comment #11)
> Created attachment 588706 [details] [diff] [review]
> v3 Fix mozglue on Linux packaging warning
> 
> I didn't need to change browser/locales/Makefile.in, but it built
> successfully, so hooray? Currently testing on the try server now:
> https://tbpl.mozilla.org/?tree=Try&rev=2ccbccaf0d7d

Actually as long as l10n builds/repacks exist, you do need to modify browser/locales/Makefile.in here as well. Unless my memory is failing me, which I don't think it is. (I'm supposed to be taking the weekend off from this stuff, so not digging right now)
Comment 15 Serge Gautherie (:sgautherie) 2012-01-14 21:07:38 PST
(In reply to Serge Gautherie (:sgautherie) from comment #12)
> ifdef MOZ_ENABLE_GNOME_COMPONENT
> DEFINES += -DMOZ_ENABLE_GNOME_COMPONENT=1
> endif

(In reply to Justin Wood (:Callek) from comment #14)
> Actually as long as l10n builds/repacks exist, you do need to modify
> browser/locales/Makefile.in here as well. Unless my memory is failing me,
> which I don't think it is. (I'm supposed to be taking the weekend off from
> this stuff, so not digging right now)

I know little about l10n.
I could only write that I don't think I ever had to update it.
http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in
At first glance, it contains only actually l10n-related code/files.

If it should be updated, it looks like it would be missing (some/all) other defines too, would it not?
Take MOZ_ENABLE_GNOME_COMPONENT as an example.
http://mxr.mozilla.org/mozilla-central/search?string=MOZ_ENABLE_GNOME_COMPONENT&case=on
Comment 16 Jon Buckley 2012-01-14 21:10:12 PST
> Nit: perfect would be if there was an existing variable to reuse, as in
> ifdef MOZ_ENABLE_GNOME_COMPONENT
> DEFINES += -DMOZ_ENABLE_GNOME_COMPONENT=1
> endif
> but is there?

I don't think there is, in my searching of MXR, and /mozglue/build/Makefile.in.

> Actually as long as l10n builds/repacks exist, you do need to modify browser/locales
> /Makefile.in here as well. Unless my memory is failing me, which I don't think it
> is. (I'm supposed to be taking the weekend off from this stuff, so not digging
> right now)

I'll see if I can figure it out
Comment 17 Serge Gautherie (:sgautherie) 2012-01-14 22:18:40 PST
(In reply to Jon Buckley [:jbuck] from comment #16)
> > Nit: perfect would be if there was an existing variable to reuse
> 
> I don't think there is, in my searching of MXR, and
> /mozglue/build/Makefile.in.

Yes, that would mean configure.in and "playing" with MOZ_MEMORY[*], and AC_DEFINE() and/or AC_SUBST().
But it's probably overkill: reviewer should tell you.
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-18 13:26:01 PST
Comment on attachment 588706 [details] [diff] [review]
v3 Fix mozglue on Linux packaging warning

Review of attachment 588706 [details] [diff] [review]:
-----------------------------------------------------------------

This is a bit hacky, but I'm ok with it if you add a comment in mozglue/build/Makefile.in explaining that the condition here needs to be updated if that's ever changed.

r=me with that
Comment 19 Serge Gautherie (:sgautherie) 2012-01-18 13:33:19 PST
Comment on attachment 588706 [details] [diff] [review]
v3 Fix mozglue on Linux packaging warning

Review of attachment 588706 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/Makefile.in
@@ +92,5 @@
>  DEFINES += -DSHIP_FEEDBACK=1
>  endif
>  
> +ifneq (,$(filter WINNT Darwin Android,$(OS_TARGET)))
> +DEFINES += -DSHARED_MOZGLUE=1

Nit: name it MOZ_SHARED_MOZGLUE.
Comment 20 Jon Buckley 2012-01-19 11:30:10 PST
Created attachment 589927 [details] [diff] [review]
v4 Fix mozglue on Linux packaging warning
[Checked in: Comment 21]

Fixed the nits
Comment 21 Serge Gautherie (:sgautherie) 2012-01-20 04:07:23 PST
Comment on attachment 589927 [details] [diff] [review]
v4 Fix mozglue on Linux packaging warning
[Checked in: Comment 21]

https://hg.mozilla.org/mozilla-central/rev/30b1cb696452
Comment 22 Serge Gautherie (:sgautherie) 2012-01-20 07:59:54 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=8693526&tree=Firefox&full=1
Linux x86-64 mozilla-central build on 2012-01-20 03:52:09 PST for push 6af8c11d727a

V.Fixed

Note You need to log in before you can comment on or make changes to this bug.