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

VERIFIED FIXED in Firefox 12

Status

()

Firefox
Build Config
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: Jon Buckley)

Tracking

Trunk
Firefox 12
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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).
}
(Reporter)

Updated

6 years ago
Blocks: 713132
(Reporter)

Updated

6 years ago
Blocks: 716395
I see this on my builds as well.
(Assignee)

Comment 2

6 years ago
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...
Assignee: nobody → jon
Status: NEW → ASSIGNED
(Reporter)

Comment 3

6 years ago
Ftr, bug 701371 renamed the file:
{
Warning: package error or possible missing or unnecessary file: bin/libmozglue.so (package-manifest, 34).
}
Depends on: 701371
Summary: [Linux] "Warning: package error or possible missing or unnecessary file: bin/libmozutils.so" → [Linux] "Warning: package error or possible missing or unnecessary file: bin/libmozglue.so (package-manifest, 34)."
(Assignee)

Comment 4

6 years ago
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.
Attachment #588592 - Flags: review?(khuey)
(Reporter)

Comment 5

6 years ago
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?
Attachment #588592 - Flags: feedback-
(Assignee)

Comment 6

6 years ago
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.
Attachment #588592 - Attachment is obsolete: true
Attachment #588592 - Flags: review?(khuey)
Attachment #588696 - Flags: review?(khuey)
Attachment #588696 - Flags: feedback?(sgautherie.bz)
(Reporter)

Comment 7

6 years ago
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.
Attachment #588696 - Flags: feedback?(sgautherie.bz)
(Reporter)

Comment 8

6 years ago
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/....
(Assignee)

Comment 9

6 years ago
> 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
(Reporter)

Comment 10

6 years ago
"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.
(Assignee)

Comment 11

6 years ago
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
Attachment #588696 - Attachment is obsolete: true
Attachment #588696 - Flags: review?(khuey)
Attachment #588706 - Flags: review?(khuey)
Attachment #588706 - Flags: feedback?(sgautherie.bz)
(Reporter)

Comment 12

6 years ago
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?
Attachment #588706 - Flags: feedback?(sgautherie.bz) → feedback+
(Reporter)

Comment 13

6 years ago
(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?
(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)
(Reporter)

Comment 15

6 years ago
(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
(Assignee)

Comment 16

6 years ago
> 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
(Reporter)

Comment 17

6 years ago
(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.
(Reporter)

Updated

6 years ago
Blocks: 713133
(Reporter)

Updated

6 years ago
Priority: -- → P1
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
Attachment #588706 - Flags: review?(khuey) → review+
(Reporter)

Comment 19

6 years ago
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.
(Assignee)

Comment 20

6 years ago
Created attachment 589927 [details] [diff] [review]
v4 Fix mozglue on Linux packaging warning
[Checked in: Comment 21]

Fixed the nits
Attachment #588706 - Attachment is obsolete: true
Attachment #589927 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Reporter)

Comment 21

6 years ago
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
Attachment #589927 - Attachment description: v4 Fix mozglue on Linux packaging warning → v4 Fix mozglue on Linux packaging warning [Checked in: Comment 21]
(Reporter)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
(Reporter)

Comment 22

6 years ago
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
Status: RESOLVED → VERIFIED

Updated

5 years ago
Blocks: 770628
You need to log in before you can comment on or make changes to this bug.