Move in-tree library linkage information to moz.build

RESOLVED FIXED in mozilla34

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks 2 bugs)

unspecified
mozilla34
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 16 obsolete attachments)

10.01 KB, patch
gps
: review+
Details | Diff | Splinter Review
6.00 KB, patch
gps
: review+
Details | Diff | Splinter Review
54.08 KB, patch
glandium
: review+
Details | Diff | Splinter Review
15.80 KB, patch
glandium
: review+
Details | Diff | Splinter Review
33.55 KB, patch
gps
: review+
Details | Diff | Splinter Review
8.36 KB, patch
gps
: review+
Details | Diff | Splinter Review
129.49 KB, patch
gps
: review+
Details | Diff | Splinter Review
38.45 KB, patch
gps
: review+
Details | Diff | Splinter Review
54.54 KB, patch
gps
: review+
Details | Diff | Splinter Review
8.60 KB, patch
gps
: review+
Details | Diff | Splinter Review
631 bytes, patch
ewong
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
The goal is double:
 - allow to have in-tree inter-dependencies known to the build backend so that it can do smarter things
 - hopefully less confusing configuration for developers
(Assignee)

Comment 1

5 years ago
At the same time, make the Library data more useful in the build frontend.
Attachment #8453711 - Flags: review?(gps)
(Assignee)

Comment 2

5 years ago
At the same time, make the Library data more useful in the build frontend.
Attachment #8453712 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453711 - Attachment is obsolete: true
Attachment #8453711 - Flags: review?(gps)
(Assignee)

Comment 7

5 years ago
Attachment #8453723 - Flags: review?(gps)
(Assignee)

Comment 8

5 years ago
This, and part 6, are what I'm after, here, as far as moz.build configuration goes.

The patch queue is not entirely tested at the moment, and incomplete, but I wanted at least a first review pass.

I haven't decided whether to keep allowing LIBRARY_NAME overlap. Now that there's STATIC_LIBRARY_NAME and SHARED_LIBRARY_NAME, we could impose all LIBRARY_NAMEs to be different. On the other hand, we could allow disambiguation by passing relative or topobjdir-absolute paths (in the form some/path/library_name, not some/path/libfoo.a).

I'm pondering what to do with the various other kinds of linkage, and I'm very tempted to allow all sorts of value in LIBS/HOST_LIBS:
- a code name corresponding to a LIBRARY_NAME (which is what this patch queue is adding)
- a code name with a relative or topobjdir-absolute path as mentioned above
- a full path to a SDK library (think android NDK, direct X import libs...)
- "-Lpath -llib" straight out of pkgconfig

The typical use for these would be to write something like:
LIBS += [
    CONFIG['MOZ_ZLIB_LIBS'],
]
etc.
and not have to care about doing checks whether we're using native zlib or not, and switch variable in that case.

I'm not sure how to handle windows system libraries, though (which need to either be -lfoo or foo.lib depending whether we build with msvc or mingw). Maybe kill OS_LIBS and have a windows-specific WIN_LIBS?
Attachment #8453729 - Flags: review?(gps)
(In reply to Mike Hommey [:glandium] from comment #8)
> I'm pondering what to do with the various other kinds of linkage, and I'm
> very tempted to allow all sorts of value in LIBS/HOST_LIBS:

This sounds nice, but my only worry is that it will be difficult to disambiguate--both by developers reading moz.build files and by the build system trying to validate entries.

Is there any prior art here in other build description languages (pants/buck/gn)?
(Assignee)

Updated

5 years ago
Attachment #8453712 - Attachment is obsolete: true
Attachment #8453712 - Flags: review?(gps)
(Assignee)

Comment 11

5 years ago
Refreshed against new part 1.
Attachment #8453743 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453717 - Attachment is obsolete: true
Attachment #8453717 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453721 - Attachment is obsolete: true
Attachment #8453721 - Flags: review?(gps)
(Assignee)

Comment 13

5 years ago
Needed to get rid of two instances of SHARED_LIBRARY_NAME in makefiles.
Attachment #8453772 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453741 - Attachment is obsolete: true
Attachment #8453741 - Flags: review?(gps)
(Assignee)

Comment 14

5 years ago
Order is important for breakpad HOST_LIBS.
Attachment #8453775 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453729 - Attachment is obsolete: true
Attachment #8453729 - Flags: review?(gps)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > I'm pondering what to do with the various other kinds of linkage, and I'm
> > very tempted to allow all sorts of value in LIBS/HOST_LIBS:
> 
> This sounds nice, but my only worry is that it will be difficult to
> disambiguate--both by developers reading moz.build files and by the build
> system trying to validate entries.
> 
> Is there any prior art here in other build description languages
> (pants/buck/gn)?

I haven't looked at the patches yet, but I thought we decided that style should take a back seat to a) conversions b) build efficiency. i.e. if we can make things look clean for little extra effort, great. If not, we won't wait on it. Given the relative stagnation in the build system lately, I'm pretty much prepared to r+ anything that looks like a step in the right direction, especially if it can lead to build time wins. Since inefficiencies around the libs tier are the most painful element now, this patch series looks very attractive.

Also, rewriting moz.build files shouldn't be too difficult. 2to3.py is a generic AST transformation tool. We can leverage that for refactoring moz.build files into prettier formats.
Comment on attachment 8453772 [details] [diff] [review]
part 1 - Move MAKE_FRAMEWORK, SDK_LIBRARY, SHARED_LIBRARY_NAME and STATIC_LIBRARY_NAME to moz.build

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

After reading the patch, I have no concerns about what the grammar looks like. This patch is obviously a step in the right direction. Defining libraries can actually be grokked by mere mortals now. I think I even learned a thing or two by reading the logic in emitter.py.

The only thing that would make this patch better is more documentation around how to define libraries. Maybe a summary of the different library options and when to use them, etc. But I'm not going to hold back r+ for that. Maybe you can write the docs while I finish reviewing this series...

::: config/rules.mk
@@ -202,5 @@
> -ifdef FORCE_SHARED_LIB
> -ifndef FORCE_STATIC_LIB
> -LIBRARY := $(NULL)
> -endif
> -endif

I will gladly trade lines of make for lines of Python.

::: mozglue/build/Makefile.in
@@ -19,5 @@
> -SDK_LIBRARY = $(REAL_LIBRARY)
> -endif
> -endif
> -
> -endif

I love how blocks like this can be replaced by a single line. So much clearer what the intent is.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +59,5 @@
>  
>  from .gyp_reader import GypSandbox
>  
>  
> +class OrderedDefaultDict(OrderedDict):

This class should ideally be in mozbuild.util.

@@ +414,5 @@
> +                raise SandboxValidationError(
> +                    'FINAL_LIBRARY conflicts with IS_FRAMEWORK', sandbox)
> +            if is_component:
> +                raise SandboxValidationError(
> +                    'FINAL_LIBRARY conflicts with IS_COMPONENT', sandbox)

Would you care to make these messages a little more actionable? e.g.

FINAL_LIBRARY implies FORCE_STATIC_LIB; remove one

@@ +421,3 @@
>  
> +        if libname:
> +            args = { 'kind': 0 }

Nit: cuddle braces

::: xpcom/glue/moz.build
@@ -97,5 @@
>  
>  LIBRARY_NAME = 'xpcomglue_s'
>  
> -SDK_LIBRARY = [
> -    "%s%s.%s" % (CONFIG['LIB_PREFIX'], 'xpcomglue_s', CONFIG['LIB_SUFFIX']),

More needless complexity abstracted away.
Attachment #8453772 - Flags: review?(gps) → review+
Attachment #8453713 - Flags: review?(gps) → review+
Attachment #8453714 - Flags: review?(gps) → review+
Comment on attachment 8453743 [details] [diff] [review]
part 4 - Allow to track library linkage for all kinds of programs and libraries

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +425,5 @@
>          is_component = sandbox.get('IS_COMPONENT')
>  
>          soname = sandbox.get('SONAME')
>  
> +        args = { 'kind': 0 }

Nit: cuddle
Attachment #8453743 - Flags: review?(gps) → review+
Comment on attachment 8453746 [details] [diff] [review]
part 5 - Hook the LIBS and HOST_LIBS moz.build variables to library linkage

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1193,5 @@
> +                                       % (relpath, lib.shared_name))
> +            elif isinstance(obj, (HostLibrary, HostProgram)):
> +                assert lib.kind == lib.STATIC
> +                backend_file.write('HOST_LIBS += %s/%s\n'
> +                                   % (relpath, lib.static_name))

Does this patch on its result in a clean build? I'd think writing these new variables would confuse rules.mk. Do we dedupe the variables or something?

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +192,5 @@
>              yield obj
>  
> +    def _link_libraries(self, sandbox, obj, variable):
> +        """Add linkage declarations to a given object.
> +        """

Nit: You don't need the extra line for the ending """

@@ +205,5 @@
> +                raise SandboxValidationError(
> +                    '%s contains "%s", which matches a %s defined in multiple '
> +                    'places:\n    %s' % (variable, name, obj.NAME_VAR,
> +                    '\n    '.join(mozpath.join(l.relativedir, 'moz.build')
> +                        for l in candidates)), sandbox)

Nit: This is a bit harder to read. Maybe extract the mozpath.join(...) into its own variable?
Attachment #8453746 - Flags: review?(gps) → review+
Comment on attachment 8453723 [details] [diff] [review]
part 6 - Move most HOST_LIBS to moz.build

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

I love how moz.build makes things so much cleaner and helps to increase understanding.
Attachment #8453723 - Flags: review?(gps) → review+
Comment on attachment 8453723 [details] [diff] [review]
part 6 - Move most HOST_LIBS to moz.build

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

Oh, did you want to ban HOST_LIBS from Makefile.in?
Comment on attachment 8453775 [details] [diff] [review]
part 7 - Move some SHARED_LIBRARY_LIBS to moz.build, as LIBS

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

I'm nearly weeping at how amazing this patch series is.

I love how we can just reference libraries by name now instead of having to know their exact paths in the build config.

I can't wait to see the next patch series. I assume that will involve light magic where we use our new library relationship knowledge to traverse the libs tier more efficiently.
Attachment #8453775 - Flags: review?(gps) → review+
Blocks: 1037058
(Assignee)

Comment 22

5 years ago
(In reply to Gregory Szorc [:gps] from comment #20)
> Oh, did you want to ban HOST_LIBS from Makefile.in?

There's still one for -lz in mozglue/linker, but I guess I could move that to HOST_LDFLAGS or HOST_EXTRA_LIBS and close the loop.

(In reply to Gregory Szorc [:gps] from comment #21)
> I can't wait to see the next patch series. I assume that will involve light
> magic where we use our new library relationship knowledge to traverse the
> libs tier more efficiently.

I have that working for host libs and host programs because all in-tree interdependencies are now declared in moz.build (except one for host_stdc++compat), while there is still a lot left to do for target libs and programs.
(Assignee)

Comment 23

5 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > I'm pondering what to do with the various other kinds of linkage, and I'm
> > very tempted to allow all sorts of value in LIBS/HOST_LIBS:
> 
> This sounds nice, but my only worry is that it will be difficult to
> disambiguate--both by developers reading moz.build files and by the build
> system trying to validate entries.

Actually, it's not a problem for the build system itself. It can be seen as a problem for developers, but I, for one, think this is a detail developers should not care about. They need a library, they say they need it. That the library is a shared library on windows and not on linux is not their problem. It's ours (build config peers). And we can easily add tools to ensure that we're not overly using static libraries and whatnot.

It can still turn out that I'm wrong in my thinking, in which case it will still be easy to change automatically. I think it's fine to try better things, even if they turn out not to be great. I don't think it's any worse than the current status-quo.
(Assignee)

Comment 24

5 years ago
(In reply to Gregory Szorc [:gps] from comment #16)
> The only thing that would make this patch better is more documentation
> around how to define libraries. Maybe a summary of the different library
> options and when to use them, etc.

What's the location for such docs?
(Assignee)

Comment 25

5 years ago
(In reply to Gregory Szorc [:gps] from comment #18)
> Comment on attachment 8453746 [details] [diff] [review]
> part 5 - Hook the LIBS and HOST_LIBS moz.build variables to library linkage
> 
> Review of attachment 8453746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1193,5 @@
> > +                                       % (relpath, lib.shared_name))
> > +            elif isinstance(obj, (HostLibrary, HostProgram)):
> > +                assert lib.kind == lib.STATIC
> > +                backend_file.write('HOST_LIBS += %s/%s\n'
> > +                                   % (relpath, lib.static_name))
> 
> Does this patch on its result in a clean build? I'd think writing these new
> variables would confuse rules.mk. Do we dedupe the variables or something?

I'm not sure what you mean here. HOST_LIBS is a StrictOrderingOnAppendList, don't we reject dupes there?

(That said, yes, in the future, we may want to add things like loop detection, but that seems overkill)
(In reply to Mike Hommey [:glandium] from comment #23)
> It can still turn out that I'm wrong in my thinking, in which case it will
> still be easy to change automatically. I think it's fine to try better
> things, even if they turn out not to be great. I don't think it's any worse
> than the current status-quo.

+1 from me on that. I haven't read the patches, but from your description here they sound like a huge improvement.
(Assignee)

Comment 27

5 years ago
One thing that /might/ lead to confusion is the variable name "LIBS", especially if we end up placing system libraries in a different one. I would like, however, to make things such that all libraries that are in-tree (third party non-moz.build-covered included) are all in a single variable, even for the case where those in-tree third party libraries can be replaced by system libraries.

Unrelatedly, there is another case where while the scheme in this patch queue is to link the shared library when there are both a static and shared library generated for a given LIBRARY_NAME, we still want to link the static library. This currently happens for the js engine library on windows, which is built as a shared library, and linked as such to libxul, but still linked statically to a bunch of other stuff. We may want to allow LIBS to contain a STATIC_LIBRARY_NAME for such cases.
(Assignee)

Comment 28

5 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26)
> but from your description, here they sound like a huge improvement.

Here is, I think, the most extreme example: 

We have this in configure.in:
MOZ_JS_STATIC_LIBS='$(call EXPAND_LIBNAME_PATH,js_static,$(LIBXUL_DIST)/lib)'
MOZ_JS_SHARED_LIBS='$(call EXPAND_LIBNAME_PATH,mozjs,$(LIBXUL_DIST)/lib)'

Then, still in configure.in, we choose one depending on whether we declared we wanted a shared js library and assign it to MOZ_JS_LIBS, which we then use in several locations, adding it to either LIBS or EXTRA_DSO_LDOPTS, depending where you look (which, in itself, is an abuse of semantics (and of the lack of enforcement in rules.mk), since static libraries linked into shared libraries are "supposed" to be in SHARED_LIBRARY_LIBS.

With the patch queue (and a few adjustments to js/src/moz.build), it would become LIBS += ['js'] (or 'mozjs', depending the name we choose).

The downside is that this is going to break --with-libxul-sdk builds, but I have no sympathy for them at the moment (and as a disclaimer, I'll point that I'm probably the one that has been using --with-libxul-sdk for the longest).
(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Gregory Szorc [:gps] from comment #16)
> > The only thing that would make this patch better is more documentation
> > around how to define libraries. Maybe a summary of the different library
> > options and when to use them, etc.
> 
> What's the location for such docs?

I'd add a "linking" page to build/docs.
(In reply to Mike Hommey [:glandium] from comment #27)
> One thing that /might/ lead to confusion is the variable name "LIBS",
> especially if we end up placing system libraries in a different one.

I will admit this confused me a bit during review. When I got to the patch that rewrote SHARED_LIBRARY_LIBS in Makefile.in to LIBS in moz.build, I had to look at the previous patches to validate my understanding matched reality.

On the subject of LIBS, perhaps a better variable name would be LINK_LIBS or LINK_AGAINST_LIBS. If we ever need to explicitly control the link type for libraries that have both static and shared forms, perhaps values could be "shared:mozjs" or "static:mozjs." If a library has two forms, we could even enforce that moz.build files are explicit about which form of linking to use, to avoid any ambiguity and surprises.
(Assignee)

Comment 31

5 years ago
> If a library has two forms, we could even enforce that moz.build files
> are explicit about which form of linking to use, to avoid any ambiguity
> and surprises.

That would make it necessary to do if CONFIG['JS_SHARED_LIBRARY'] when linking the js library. That's why I suggested to use the STATIC_LIBRARY_NAME, because that never changes (although static:mozjs would work too).
(Assignee)

Comment 32

5 years ago
> On the subject of LIBS, perhaps a better variable name would be LINK_LIBS or LINK_AGAINST_LIBS.

Note that when I originally toyed with the idea behind this bug, the variable was LINKED_LIBRARIES. But that seemed too verbose, and I stumbled upon LIBS in sandbox_symbols.py. LINK_LIBS sounds good.
(Assignee)

Comment 33

5 years ago
jcranmer suggested USED_LIBS, which rubbed me in the right way, and led me to USE_LIBS.

Pick your bikeshed color.
I'm fine with USE_LIBS.
(Assignee)

Updated

5 years ago
Depends on: 1038639
(Assignee)

Comment 35

5 years ago
At the same time, make the Library data more useful in the build frontend.
(Assignee)

Updated

5 years ago
Attachment #8453772 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
Comment on attachment 8457969 [details] [diff] [review]
part 1 - Move MAKE_FRAMEWORK, SDK_LIBRARY, SHARED_LIBRARY_NAME and STATIC_LIBRARY_NAME to moz.build

Addressed review comments, so carrying r+ over.
Attachment #8457969 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8453743 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
Comment on attachment 8457970 [details] [diff] [review]
part 4 - Allow to track library linkage for all kinds of programs and libraries

Addressed review comments.
Attachment #8457970 - Flags: review+
(Assignee)

Comment 39

5 years ago
Significant changes in here, to support changing more things in part 7 (now 8). This adds the discussed static: prefix. Part 7 and 8 clue in as to what moz.build definitions look like, until I write proper documentation.
Attachment #8457973 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453746 - Attachment is obsolete: true
(Assignee)

Comment 40

5 years ago
Comment on attachment 8457973 [details] [diff] [review]
part 5 - Hook the USE_LIBS and HOST_USE_LIBS moz.build variables to library linkage

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +103,5 @@
>          self.fh.write(buf)
>  
> +    def write_once(self, buf):
> +        if '\n' + buf not in self.fh.getvalue():
> +            self.write(buf)

Note this is an awful hack, but necessary to avoid repeating library related stuff when there are several programs or libraries in a directory (avoiding the repetition avoids build failure). It could be done differently, but that would involve much more code, and I didn't feel it was worth.
(Assignee)

Comment 41

5 years ago
Note the sandbox_symbols.py description is essentially a copy/paste from the one for SIMPLE_PROGRAMS.
Attachment #8457978 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8453723 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8453775 - Attachment is obsolete: true
(Assignee)

Comment 44

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=063b20dcdef7

From there, the plan is to wait for parts 9 and 10, and land after next merge day.

Part 9 will move SHARED_LIBRARY_LIBS, EXTRA_DSO_LDOPTS and LIBS to OS_LIBS or EXTRA_LIBS in Makefiles, and forbid them.
Part 10 will add the much awaited documentation.

More will come in followup bugs.
(Assignee)

Comment 45

5 years ago
With more that I spotted when changing variables names for part 9.
Attachment #8458383 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8457980 - Attachment is obsolete: true
Attachment #8457980 - Flags: review?(gps)
(Assignee)

Comment 46

5 years ago
Forgot mozgtk_stub.
Attachment #8458384 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8458383 - Attachment is obsolete: true
Attachment #8458383 - Flags: review?(gps)
(Assignee)

Comment 47

5 years ago
Gah, missed some more.
Attachment #8458393 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8458384 - Attachment is obsolete: true
Attachment #8458384 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Depends on: 1040639
(Assignee)

Updated

5 years ago
Depends on: 1040641
(Assignee)

Updated

5 years ago
Attachment #8458393 - Attachment is obsolete: true
Attachment #8458393 - Flags: review?(gps)
(Assignee)

Comment 49

5 years ago
With more sanity check to avoid conflicts between import library file name and static library file name.
Attachment #8458589 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #8457973 - Attachment is obsolete: true
Attachment #8457973 - Flags: review?(gps)
(Assignee)

Comment 50

5 years ago
OS_LIBS for libraries that are not part of the gecko tree, EXTRA_LIBS for
libraries, such as NSPR, that are in the tree, but are not handled by
moz.build just yet. Those EXTRA_LIBS may also come from a system library.
However, in cases where the expanded variables are always empty for the
in-tree case, OS_LIBS is used (as for, e.g. MOZ_ZLIB_LIBS). OS_LDFLAGS is
used exclusively for non-library linker flags.

Always pass EXTRA_LIBS before OS_LIBS on linker command lines.

Forbid EXTRA_DSO_LDOPTS, SHARED_LIBRARY_LIBS and LIBS in Makefiles.

https://tbpl.mozilla.org/?tree=Try&rev=90c11a5fa894
Attachment #8458595 - Flags: review?(gps)
(Assignee)

Comment 51

5 years ago
This purposefully doesn't cover host programs and libraries. They are rare enough that this would only add noise, IMHO.
Attachment #8458613 - Flags: review?(gps)
Attachment #8458589 - Flags: review?(gps) → review+
Comment on attachment 8457978 [details] [diff] [review]
part 6 - Emit SimplePrograms for CPP_UNIT_TESTs, and make the corresponding moz.build config look like that of SIMPLE_PROGRAMS

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

Will grant r+ if you convince me the pattern noted in the other comment must exist.

::: content/base/test/moz.build
@@ +23,3 @@
>  ]
>  
> +SOURCES += sorted('%s.cpp' % t for t in CPP_UNIT_TESTS)

This pattern is pretty annoying for moz.build authors.

Can we not perform this operation (or similar) inside emitter.py?
Attachment #8457978 - Flags: review?(gps) → review-
Attachment #8457979 - Flags: review?(gps) → review+
Attachment #8458613 - Flags: review?(gps) → review+
Comment on attachment 8458595 [details] [diff] [review]
part 9 - Replace all EXTRA_DSO_LDOPTS, SHARED_LIBRARY_LIBS and LIBS with EXTRA_LIBS, OS_LIBS or OS_LDFLAGS, appropriately

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

Do you have any plans to move EXTRA_LIBS, OS_LIBS, and OS_LDFLAGS to moz.build? If nothing else, we can validate that libraries in these aren't a library known to moz.build.
Attachment #8458595 - Flags: review?(gps) → review+
Comment on attachment 8458535 [details] [diff] [review]
part 8 - Move most in-tree library linkage information to moz.build, as USE_LIBS

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

I'm not a huge fan of expanding XPCOM_GLUE_LDOPTS into |USE_LIBS += ['mozalloc', 'xpcomglue_s', 'xul-shared']|. I'd almost rather have a shorthand in moz.build for declaring that. Will the build fail in obvious ways if e.g. mozalloc or xpcomglue_s are omitted from that list? Could we get silent successes? I reckon that can be deferred to a follow-up.

I glanced over large parts of this patch that looked like cookie cutter changes. I did that because bugs in linking configuration should result in obvious build failures.
Attachment #8458535 - Flags: review?(gps) → review+
(Assignee)

Comment 55

5 years ago
(In reply to Gregory Szorc [:gps] from comment #52)
> Comment on attachment 8457978 [details] [diff] [review]
> part 6 - Emit SimplePrograms for CPP_UNIT_TESTs, and make the corresponding
> moz.build config look like that of SIMPLE_PROGRAMS
> 
> Review of attachment 8457978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Will grant r+ if you convince me the pattern noted in the other comment must
> exist.

I won't claim it must exist, but here I'm just looking for consistency with SIMPLE_PROGRAMS (note how the same code is used for both cases). I'm all for changing that, but I'd rather do that in a followup.

(In reply to Gregory Szorc [:gps] from comment #54)
> I'm not a huge fan of expanding XPCOM_GLUE_LDOPTS into |USE_LIBS +=
> ['mozalloc', 'xpcomglue_s', 'xul-shared']|. I'd almost rather have a
> shorthand in moz.build for declaring that.

I'm not a big fan either, but after having written this patch, I'm not a big fan of something like XPCOM_GLUE_LDOPTS either: there were so many places where we actually ended up with xpcomglue_s and xul (or other things) duplicated on the link command line. There are, however, one or two things I want to add that would alleviate this:
- allow intermediate static libs to have shared library dependencies, which would be linked to the final lib (and throw errors if the static lib is linked nowhere where those shared library deps are going to be used)
- define types of programs/shared libraries. Something to say "this is something for gecko", "this is independent", "this is a program with xpcom standalone linkage", etc. which would also replace e.g. resetting MOZ_GLUE_LDFLAGS and/or setting DISABLE_STL_WRAPPING
but I haven't formalized anything yet.

> Will the build fail in obvious ways if e.g. mozalloc or xpcomglue_s
> are omitted from that list? Could we get silent successes? I reckon
> that can be deferred to a follow-up.

I don't think we can get silent successes. You'd get linkage failures (missing symbols) if a lib is missing.
(Assignee)

Comment 56

5 years ago
> there were so many places where we actually ended up with xpcomglue_s and
> xul (or other things) duplicated on the link command line

Also, I'm pretty sure there are plenty of those that were only cargo-culting, and are actually useless.
(Assignee)

Updated

5 years ago
Blocks: 1041839
(In reply to Mike Hommey [:glandium] from comment #55)
> I don't think we can get silent successes. You'd get linkage failures
> (missing symbols) if a lib is missing.

I am mostly concerned with libraries like mozjemalloc that offer a drop-in replacement for existing APIs. Presumably an omission could result in linking against a seemingly compatible library.

Anyway, your comments placated me. I'll grant r+ momentarily.
Comment on attachment 8457978 [details] [diff] [review]
part 6 - Emit SimplePrograms for CPP_UNIT_TESTs, and make the corresponding moz.build config look like that of SIMPLE_PROGRAMS

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

Switching to r+ since glandium kinda/sorta has plans to make this suck less.
Attachment #8457978 - Flags: review- → review+
(Assignee)

Comment 59

5 years ago
(In reply to Gregory Szorc [:gps] from comment #57)
> (In reply to Mike Hommey [:glandium] from comment #55)
> > I don't think we can get silent successes. You'd get linkage failures
> > (missing symbols) if a lib is missing.
> 
> I am mostly concerned with libraries like mozjemalloc that offer a drop-in
> replacement for existing APIs. Presumably an omission could result in
> linking against a seemingly compatible library.

Note it's mozalloc, not mozjemalloc. Mozjemalloc is in mozglue and that's not even handled by USE_LIBS yet. Mozalloc doesn't expose drop-in replacements, it exposes moz_* functions.
(Assignee)

Updated

5 years ago
Blocks: 1041936
(Assignee)

Updated

5 years ago
Blocks: 1041941
(Assignee)

Updated

5 years ago
Blocks: 1042503
(Assignee)

Updated

5 years ago
Blocks: 882908
Depends on: 1042685

Updated

5 years ago
Blocks: 1043019
(Assignee)

Updated

5 years ago
Depends on: 1043084
Blocks: 1043045
Got this with a build for SeaMonkey:

    Traceback (most recent call last):
      File "./config.status", line 978, in <module>
        config_status(**args)
      File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/config_status.py", line 148, in config_status
        summary = the_backend.consume(definitions)
      File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/backend/base.py", line 186, in consume
        for obj in objs:
      File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/frontend/emitter.py", line 122, in emit
        objs = list(self.emit_from_sandbox(out))
      File "/builds/slave/c-cen-t-lnx/build/mozilla/python/mozbuild/mozbuild/frontend/emitter.py", line 595, in emit_from_sandbox
        'Please remove the latter.', sandbox)
    mozbuild.frontend.reader.SandboxValidationError:
    ==============================
    ERROR PROCESSING MOZBUILD FILE
    ==============================
     
    The error occurred while processing the following file or one of the files it includes:
     
        /builds/slave/c-cen-t-lnx/build/mozilla/extensions/gnomevfs/moz.build
     
    The error occurred when validating the result of the execution. The reported error is:
     
        IS_COMPONENT implies FORCE_SHARED_LIB. Please remove the latter.
     
     
    configure: error: /builds/slave/c-cen-t-lnx/build/mozilla/configure failed for mozilla
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8466568 - Flags: review?(mh+mozilla)
Comment on attachment 8466568 [details] [diff] [review]
gnomevfs moz.build change fix.

r+ from glandium over irc.
Attachment #8466568 - Flags: review?(mh+mozilla) → review+
Status: REOPENED → ASSIGNED
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 66

5 years ago
Don't reopen for a fixup that doesn't even affect m-c builds.
QA Whiteboard: [qa-]
Depends on: 1107063

Updated

4 years ago
Depends on: 1112904

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.