Closed Bug 1000994 Opened 6 years ago Closed 6 years ago

Allow to specify a library SONAME

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: pochu27, Assigned: mukilanthiagarajan, Mentored)

References

Details

(Whiteboard: [good next bug][lang=python][lang=make])

Attachments

(2 files, 6 obsolete files)

In bug #624422, we're creating various libraries with the same SONAME but different name, overriding -Wl,-h in LDFLAGS for that. In comment #67 there, Mike proposed:

> Ideally, we would avoid the original -Wl,-h to be passed on the command line
> from MKSHLIB. Ideally, we'd define a SONAME here (or moz.build), that
> defaults to $(notdir $@) in config/rules.mk, and change the various MKSHLIB
> definitions in configure.in. That could probably be done in a followup bug.
Whiteboard: [good first bug][mentor=glandium][lang=python][lang=make]
Moving to good-next - this looks heavy for a starter bug. Mike, thanks for your ongoing awesomeness flagging and tagging these.
Whiteboard: [good first bug][mentor=glandium][lang=python][lang=make] → [good next bug][mentor=glandium][lang=python][lang=make]
Mentor: mh+mozilla
Whiteboard: [good next bug][mentor=glandium][lang=python][lang=make] → [good next bug][lang=python][lang=make]
I'd like to work on this bug. Can someone please assign it to me?
We don't usually assign bugs to new bugzilla users (in the sense of the "Assigned To" field), but feel free to work on it. If you need assistance, please hop on irc, in the #build channel, or ask your questions here (but it's usually better on irc)
This is first patch in a series of two. It allows specifying SONAME in moz.build file. If no SONAME is specified, gnu ld by defaults uses the filename of this library as the SONAME in the binaries that link to this library. 

Also, only linux platform is supported for now. I haven't looked into how other *nix platforms deal with SONAME.
Attachment #8450017 - Flags: review?(mh+mozilla)
This is the second patch and final patch in the series. 

The current implementation of the solution outlined in https://bugzilla.mozilla.org/show_bug.cgi?id=624422#c39 uses LDFLAGS in Makefile.in to specify the SONAME. This patch uses the new SONAME option in moz.build to do the same, and removes the LDFLAGS manipulation from Makefile.in.

Note, however, that the Makefile.in is still needed to specify the --no-as-needed flag. Should this be moved to moz.build as well?
Attachment #8450031 - Flags: review?(mh+mozilla)
I realized that the Makefile.in of the stub library is empty a. This patch essentially deletes Makefile.in from widget/gtk/mozgtk/stub directory
Attachment #8450031 - Attachment is obsolete: true
Attachment #8450031 - Flags: review?(mh+mozilla)
Attachment #8450034 - Flags: review?(mh+mozilla)
Fixed an incorrect key lookup. Also, I will add the tests once the basic design is approved
Attachment #8450017 - Attachment is obsolete: true
Attachment #8450017 - Flags: review?(mh+mozilla)
Attachment #8450159 - Flags: review?(mh+mozilla)
Comment on attachment 8450159 [details] [diff] [review]
0001-Bug-1000994-Allow-specifying-SONAME-for-shared-libs-.patch

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

The approach is sound, but you'll need some changes before landing this, see below. Note for the future, please avoid the [PATCH] headers in the patch summary. (they're not stripped when importing in mercurial)

::: config/rules.mk
@@ +495,5 @@
>  EXTRA_DSO_LDOPTS += -Wl,--version-script,$(LD_VERSION_SCRIPT)
>  EXTRA_DEPS += $(LD_VERSION_SCRIPT)
>  endif
> +ifdef SONAME
> +EXTRA_DSO_LDOPTS += -Wl,-soname,$(DLL_PREFIX)$(SONAME)$(DLL_SUFFIX)

This needs to be done in configure.in and js/src/configure.in instead. Check all the $(notdir $@) things in the various MKSHLIB/MKCSHLIB values.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +386,5 @@
>  
> +        soname = sandbox.get('SONAME')
> +        if soname:
> +            if final_lib or sandbox.get('FORCE_STATIC_LIB'):
> +                raise SandboxValidationError('SONAME applicable only for shared libaries')

The error message should be different for the final_lib case, because the final_lib might actually be a shared library.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +409,5 @@
> +        """The soname of the shared object currently being linked
> +
> +        soname is the "logical name" of a shared object, often used to provide
> +        version backwards compatibility. This variable only makes sense for
> +        shared objects, and currently supported only on Linux.

In practice, that's Linux, BSDs and probably others.
Attachment #8450159 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8450034 [details] [diff] [review]
0002-Bug-1000994-Use-SONAME-to-implement-solution-propose.patch

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

Please change the patch summary to be self-describing (instead of referring to something written in comments in a bug).
Attachment #8450034 - Flags: review?(mh+mozilla) → review+
Thanks for the review Mike. I've made the changes as per your feedback.
Attachment #8450159 - Attachment is obsolete: true
Attachment #8452116 - Flags: review?(mh+mozilla)
I have changed the patch summary to include more details.
Attachment #8450034 - Attachment is obsolete: true
Attachment #8452117 - Flags: review?(mh+mozilla)
Comment on attachment 8452116 [details] [diff] [review]
Rev2 - Allow SONAME in moz.build files

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

You're missing the config.mk changes to avoid SONAME being defined in Makefile.in files.

::: config/rules.mk
@@ +180,5 @@
> +ifdef SONAME
> +DSO_SONAME			= $(DLL_PREFIX)$(SONAME)$(DLL_SUFFIX)
> +else
> +DSO_SONAME			= $(notdir $@)
> +endif 

You have a trailing whitespace here.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +409,5 @@
> +        """The soname of the shared object currently being linked
> +
> +        soname is the "logical name" of a shared object, often used to provide
> +        version backwards compatibility. This variable makes sense only for
> +        shared objects, and is supported only on Linux, NetBSD and Solaris.

There's actually more than that list of OSes. You should just say something like "some unix platforms".
Attachment #8452116 - Flags: review?(mh+mozilla) → feedback+
Attachment #8452117 - Flags: review?(mh+mozilla) → review+
Attachment #8452917 - Attachment description: R → Rev 3 Allow SONAME in moz.build files (added missing changes)
Attachment #8452917 - Attachment is obsolete: true
Thanks for the review Mike. I've added the missing changes to config.mk. I forgot to include them in the previous patch. I've also fixed the other issues you pointed out.
Attachment #8452116 - Attachment is obsolete: true
Attachment #8452923 - Flags: review?(mh+mozilla)
Attachment #8452923 - Flags: review?(mh+mozilla) → review+
Attachment #8452117 - Attachment description: Rev 2 Use SONAME to specify soname of libmozgtk & stub → part 2. Use SONAME to specify soname of libmozgtk & stub
Attachment #8452923 - Attachment description: Rev 3 Allow SONAME in moz.build files (added missing changes) → part 1. Allow SONAME in moz.build files (added missing changes)
https://hg.mozilla.org/mozilla-central/rev/5e565152b016
https://hg.mozilla.org/mozilla-central/rev/c23d678b51c8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> https://hg.mozilla.org/mozilla-central/rev/5e565152b016

This one caused a bustage. Bug 1036747
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.