716397 - [Linux] "Warning: package error or possible missing or unnecessary file: bin/libmozglue.so (package-manifest, 34)."

Status: VERIFIED FIXED

(Firefox Build System :: General, defect, P1)

Description

[Serge Gautherie (:sgautherie)]

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]]

I see this on my builds as well.

Comment 2

[Jon Buckley]

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)]

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]

Attachment: 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)]

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]

Attachment: 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)]

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)]

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]

> 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)]

"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]

Attachment: 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)]

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)]

(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)]

(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)]

(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]

> 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)]

(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 (Exited; not receiving bugmail, old account, do not use)]

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)]

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]

Attachment: v4 Fix mozglue on Linux packaging warning [Checked in: Comment 21]

Fixed the nits

Comment 21

[Serge Gautherie (:sgautherie)]

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)]

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