Closed Bug 1059113 Opened 5 years ago Closed 5 years ago

Use templates for shared libraries and frameworks

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

No description provided.
Also force to use the existing template for XPCOM components.
Attachment #8479665 - Flags: review?(mshal)
Comment on attachment 8479665 [details] [diff] [review]
Use templates for shared libraries and frameworks

>diff --git a/config/external/nss/moz.build b/config/external/nss/moz.build
>--- a/config/external/nss/moz.build
>+++ b/config/external/nss/moz.build

>-Library('nss')
...
>+    Library('nss')
>+    SharedLibrary('nss')
>+    Library('nss')

It seems like a step backward here to go from listing the library name once to now listing it 2 or 3 times in some cases. What is the overall goal of this patch?

>diff --git a/toolkit/library/gtest/moz.build b/toolkit/library/gtest/moz.build
>--- a/toolkit/library/gtest/moz.build
>+++ b/toolkit/library/gtest/moz.build
>@@ -1,15 +1,13 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
>-Library('xul-gtest')
>+Libxul('xul-gtest')
> 
> FINAL_TARGET = 'dist/bin/gtest'
> 
> USE_LIBS += [
>     'static:xul',
> ]
>-
>-include('../libxul.mozbuild')
>diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build
>--- a/toolkit/library/moz.build
>+++ b/toolkit/library/moz.build
>@@ -1,15 +1,63 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
>-Library('xul')
>+@template
>+def Libxul(name):

I really don't like how gtest/moz.build no longer explicitly includes the xul definition, but instead requires the template to be defined in a separate moz.build file. You're essentially forcing the mozbuild processor to always parse every moz.build file before doing anything. Although that's what we do now, we can do better by only parsing moz.build files that have changed. That will be much harder to do with a construct like this.

What's the benefit of making this a template? Including a file is straightforward and easy to understand. This is not.
Attachment #8479665 - Flags: review?(mshal) → feedback-
(In reply to Michael Shal [:mshal] (no reviews 8/29 - 9/8) from comment #2)
> Comment on attachment 8479665 [details] [diff] [review]
> Use templates for shared libraries and frameworks
> 
> >diff --git a/config/external/nss/moz.build b/config/external/nss/moz.build
> >--- a/config/external/nss/moz.build
> >+++ b/config/external/nss/moz.build
> 
> >-Library('nss')
> ...
> >+    Library('nss')
> >+    SharedLibrary('nss')
> >+    Library('nss')
> 
> It seems like a step backward here to go from listing the library name once
> to now listing it 2 or 3 times in some cases. What is the overall goal of
> this patch?

The goal is bug 1059129 and other things that apply to shared libraries and not static libraries.

> >diff --git a/toolkit/library/gtest/moz.build b/toolkit/library/gtest/moz.build
> >--- a/toolkit/library/gtest/moz.build
> >+++ b/toolkit/library/gtest/moz.build
> >@@ -1,15 +1,13 @@
> > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > # vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> >-Library('xul-gtest')
> >+Libxul('xul-gtest')
> > 
> > FINAL_TARGET = 'dist/bin/gtest'
> > 
> > USE_LIBS += [
> >     'static:xul',
> > ]
> >-
> >-include('../libxul.mozbuild')
> >diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build
> >--- a/toolkit/library/moz.build
> >+++ b/toolkit/library/moz.build
> >@@ -1,15 +1,63 @@
> > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > # vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # License, v. 2.0. If a copy of the MPL was not distributed with this
> > # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> >-Library('xul')
> >+@template
> >+def Libxul(name):
> 
> I really don't like how gtest/moz.build no longer explicitly includes the
> xul definition, but instead requires the template to be defined in a
> separate moz.build file. You're essentially forcing the mozbuild processor
> to always parse every moz.build file before doing anything. Although that's
> what we do now, we can do better by only parsing moz.build files that have
> changed. That will be much harder to do with a construct like this.
> 
> What's the benefit of making this a template? Including a file is
> straightforward and easy to understand. This is not.

The alternative is to set a variable before including a file, which is not necessarily much better. Also note that in the future, the gtest subdirectory will go away because we'll be able to define both libraries in the same file.
Flags: needinfo?(mshal)
> You're essentially forcing the mozbuild processor to always parse every moz.build
> file before doing anything. Although that's what we do now, we can do better by
> only parsing moz.build files that have changed. 

I forgot to comment about this: it's more than what we do now. It's already mandatory to always parse every moz.build file before doing anything.

> That will be much harder to do with a construct like this.

Note harder than making it work with the currently existing templates, which would have to be done anyways.
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #3)
> (In reply to Michael Shal [:mshal] (no reviews 8/29 - 9/8) from comment #2)
> > >+    Library('nss')
> > >+    SharedLibrary('nss')
> > >+    Library('nss')
> > 
> > It seems like a step backward here to go from listing the library name once
> > to now listing it 2 or 3 times in some cases. What is the overall goal of
> > this patch?
> 
> The goal is bug 1059129 and other things that apply to shared libraries and
> not static libraries.

Ahh, ok. Is there any way to avoid the string duplication?

> The alternative is to set a variable before including a file, which is not
> necessarily much better. Also note that in the future, the gtest
> subdirectory will go away because we'll be able to define both libraries in
> the same file.

Maybe if we are able to separate out the module name from the Framework()/SharedLibrary()/etc templates, that would avoid this problem as well. Then you could just use an explicit included file, rather than relying on @templates to transfer between independent moz.build files. In any case, I'd prefer the variable+included file over @template magic in order to have a more efficient parsing model in the future. If the gtest subdirectory is going away anyway, you won't have to worry about it for long.

> I forgot to comment about this: it's more than what we do now. It's already mandatory to always
> parse every moz.build file before doing anything.

The first time you do a build, sure. But if you change a single moz.build file (such as toolkit/library/gtest/moz.build), you should be able to update the backend by only parsing toolkit/library/gtest/moz.build and the files in python/mozbuild/, completely skipping the other 1000+ moz.build files in the tree. Similarly, if you change toolkit/library/libxul.mozbuild, you should be able to only parse the two moz.build files that include it. In this case, when we read gtest/moz.build, how would it know what the Libxul template is?

Right now we parse every moz.build file when any single one changes, but there's no reason to force us into this inefficient model. If you've been making this assumption elsewhere, please revisit those cases.
Flags: needinfo?(mshal)
> Right now we parse every moz.build file when any single one changes, but there's no reason
> to force us into this inefficient model. If you've been making this assumption elsewhere,
> please revisit those cases.

My point is that we are *already* in this inefficient model. With or without this addition of a template in toolkit/library. If we ever make it out of the inefficient model, it would be fixed with the template in toolkit/library anyways. That said, even if we can skip loading unchanged moz.build, backend generation still requires the data from all of them, and backend generation takes longer than loading moz.build.
I say, let's bring a third party to the conversation.
Flags: needinfo?(gps)
moz.build processing takes <5s on modern hardware. I'm not concerned with partial moz.build processing as an optimization area at this time. Functionality first, optimization later. If this conflicts with Tup, we can always refactor how templates are implemented: I'm not too concerned that the current model is constraining us.

I would likely r+ this patch if asked. I do share the same feelings that it is unfortunate some strings/patterns are repeated in the new approach. We can bikeshed on syntax improvements in the future. These templates are a net usability win and perfect is the enemy of progress.
Flags: needinfo?(gps)
08:37 <glandium> gps: so, can i take https://bugzilla.mozilla.org/show_bug.cgi?id=1059113#c8 as an r+ and land?
08:38 <gps> glandium: yes
https://hg.mozilla.org/mozilla-central/rev/8b5e3ba0f83d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.