Implement autoconf substitution in Python

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: glandium)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [buildfaster:?])

Attachments

(10 attachments, 13 obsolete attachments)

2.82 KB, patch
ted
: review+
Details | Diff | Splinter Review
5.11 KB, patch
ted
: review+
Details | Diff | Splinter Review
450 bytes, patch
ted
: review+
Details | Diff | Splinter Review
2.71 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.17 KB, patch
ted
: review+
Details | Diff | Splinter Review
78.06 KB, patch
Details | Diff | Splinter Review
114.87 KB, patch
ted
: review+
Details | Diff | Splinter Review
72.37 KB, patch
ted
: review+
Details | Diff | Splinter Review
792.88 KB, patch
Details | Diff | Splinter Review
2.15 KB, text/plain
Details
(Reporter)

Description

5 years ago
Created attachment 642326 [details] [diff] [review]
Implement make file variable substitution in Python

I'd like to nuke acoutput-fast.pl.

There are a couple of reasons (in no particular order):

* It contains a lot of legacy code that we don't use (about 30 lines of path normalization is just not used AFAICT - I find that hard to believe though)
* It is highly specialized for substitution of just the two variables {top_srcdir, srcdir}.
* It isn't implemented as a library and some may want an API for performing variable substitution (as part of writing an alternative build system, for example).
* There are no tests.
* It is written in Perl. Python is our preferred tools language at this time.

All these point me to arrive at the conclusion that it should be reimplemented in Python. Personally, I want it for alternative build systems.

The attached patch implements generic substitution in Python. This is implemented in build/pylib/mozbuild/mozbuild/makefile/makefile.py as part of a newly-introduced, generic Makefile interaction API. I have plans for this class in the future.

There is also build/pylib/mozbuild/mozbuild/makefile/conversion.py. This is where acoutput-fast.pl is reimplemented in Python. It's built on top of the generic substitution API. It's also implemented as a library (because everything as a library is generally a good idea).

conversion.py also includes a __main__ so it can be executed directly. It more or less behaves like acoutput-fast.pl. The big difference is it takes arguments on the command line instead of inferring them. Explicitness is better than heuristics.

Of course everything has unit tests. There could be more coverage for the acoutput-fast.pl logic. But, we have no coverage for that today beside the build itself, so something is better than nothing.

I built this on top of the mozbuild "platform" from bug 751795. We'll obviously need to wait on that to land before commit. This could get bitrotted.
Attachment #642326 - Flags: review?(mh+mozilla)
(Reporter)

Comment 1

5 years ago
Created attachment 642327 [details] [diff] [review]
Replace acoutput-fast.pl in main configure

This swaps in Python over acoutput-fast.pl in the main configure.in. This can't be checked in because it will require Python 2.6 (I /think/). But, it demonstrates how this would work.

I put my objdir under a Git repository and verified that output after clobber configure is not different with this used.

Execution speed is no worse than Perl. It could be faster, but the variance in execution time is all over the map. I can say with certainty it is no slower.

The ideal patch would remove acoutput-fast.pl from js/src/configure.in and other places. However, this would require 1) mozbuild being installed in the Python virtualenv 2) the virtualenv being configured before makefile generation. And, since virtualenv is being built via a Makefile, that could be a bit troubling. At some point we'll need to change how virtualenv is configured so it can be brought up (with mozbuild) earlier in the build process. That's for another bug, which can block actually making this change.
Assignee: nobody → gps
Status: NEW → ASSIGNED
(Reporter)

Comment 2

5 years ago
I should also mention that once bug 742795 lands, we can probably remove all AC_OUTPUT from configure if we really wanted to (or at least everything except whatever produces the file containing all the variables).

I am going to advocate for this because I want configure to have as few side-effects as possible, at least in terms of things that have side-effects for the build backend. The reason is support for alternative build system. I'd like configure to perform... just configuration, where that is defined as inspecting the world and figuring out settings. e.g. no makefile generation. Whatever drives configure can invoke makefile generation (or whatever build backend is in use).
(Assignee)

Comment 3

5 years ago
(In reply to Gregory Szorc [:gps] from comment #2)
> Whatever drives configure can invoke makefile generation (or
> whatever build backend is in use).

I strongly disagree here.
(Reporter)

Comment 4

5 years ago
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Gregory Szorc [:gps] from comment #2)
> > Whatever drives configure can invoke makefile generation (or
> > whatever build backend is in use).
> 
> I strongly disagree here.

I'm going to argue that if we want to support parallel, alternative build systems, we're going to need a way to disconnect configuration/configure from Makefile.in -> Makefile generation. At the least, we need to support a 1:N mapping. Because if we don't, it makes the case of supporting "wacky" Makefile generation (e.g. rewriting or file consolidation) a bit more challenging (but not impossible).

I see the following options:

1) Disconnect Makefile generation from configure completely. Configuration and build backend "configuration" are thus loosely coupled. One doesn't imply the other. I'm not saying no make files can get generated - just Makefiles. Things like autoconf.mk can still be generated by configure.

2) Make Makefile generation optional inside configure. If an environment variable or something is defined, you just don't do it. Presumably the alternate build system drives configure and sets this.

3) Require configure to support the conversion to every supported build backend. To hack on alternative build backends thus requires hacking on configure to support them. In my opinion, this is the least attractive because configure.in is a steaming pile and nobody wants to be required to grok it to figure out how to hack on alternative build backends. Maybe if you make the docs really good or provide some kind of hooks mechanism...

I most favor #1 because I tend to favor loosely coupled systems. The emancipation of makefile generation logic from configure would also give us the opportunity to reconsider how we approach it. There is no shortage of warts we can clean up here.

If we go with #2, expect a bug from me that requests we separate Makefile make files from other autoconf output files, as we'll likely only want to disable the former since we need the latter to derive the former (assuming Makefiles are the end state - which they could be).

Do you still hold the same opinion? Can you elaborate?
(Assignee)

Comment 5

5 years ago
3) doesn't require hacking on configure. The configure part can just be calling a python script like your patch here does. The only configure part would be a flag to choose what build system to use.
(Reporter)

Comment 6

5 years ago
(In reply to Mike Hommey [:glandium] from comment #5)
> 3) doesn't require hacking on configure. The configure part can just be
> calling a python script like your patch here does. The only configure part
> would be a flag to choose what build system to use.

That's fair. But that still necessitates moving makefile generation out of configure. Or, are you advocating retaining support for makefiles in configure and making everything else a one-off via e.g. --disable-makefile-generation --backend=/path/to/script?
(Assignee)

Comment 7

5 years ago
(In reply to Gregory Szorc [:gps] from comment #6)
> That's fair. But that still necessitates moving makefile generation out of
> configure. Or, are you advocating retaining support for makefiles in
> configure and making everything else a one-off via e.g.
> --disable-makefile-generation --backend=/path/to/script?

In practice, except for the acoutput-fast hack, makefile generation is not /really/ part of configure.
What really happens is: configure creates config.status, and runs it. config.status is actually the one doing makefile generation.
What I think we should do is have an option to configure to choose the backend to use for config.status. config.status could then just be a simple wrapper that would call one of the python scripts that generates makefile/tupfiles/whatever, or several (someone may want to generate files e.g. for MSVC *and* tup), or re-run configure with the same options (config.status --recheck).
(Assignee)

Comment 8

5 years ago
FYI, modifying config.status to do the right thing was my plan for bug 742795 and this bug is actually helping to go towards that.
(Reporter)

Comment 9

5 years ago
(In reply to Mike Hommey [:glandium] from comment #8)
> FYI, modifying config.status to do the right thing was my plan for bug
> 742795 and this bug is actually helping to go towards that.

\o/

I don't fully grok how you will get config.status to do the right thing. But, I trust you know what you are doing.

As long as configure can output a machine readable map of all the variables (autoconf.mk?), the "CONFIG_FILES" list (things that define the build config) and we have a way of easily completely controlling what is done with these (e.g. if the data can be passed or piped to an arbitrary script) I think I'll be happy.

Tell me if you need anything from Python land and I can help you out.
(Reporter)

Updated

5 years ago
Blocks: 774108
No longer depends on: 751795
(Assignee)

Comment 10

5 years ago
Comment on attachment 642326 [details] [diff] [review]
Implement make file variable substitution in Python

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

::: build/pylib/mozbuild/mozbuild/makefile/conversion.py
@@ +1,5 @@
> +# 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/.
> +
> +import os.path

Since you import os, you don't need to import os.path.

@@ +7,5 @@
> +
> +from mozbuild.makefile.makefile import Makefile
> +
> +class MakefileSubstitutor(object):
> +    """Helps performs variable substitutions in Makefile's.

Since I'd like to make this the base for a config.status replacement, you might as well not make it Makefile specific (we also AC_OUTPUT non makefiles)

@@ +55,5 @@
> +            'top_srcdir': self._top_source_directory,
> +        }
> +
> +        for output_leaf in output_leafs:
> +            if not output_leaf.endswith('Makefile') or ':' in output_leaf:

os.path.basename(output_leaf) == 'Makefile'

Why the ':' in output_leaf test? I'd expect this to skip everything on windows

@@ +59,5 @@
> +            if not output_leaf.endswith('Makefile') or ':' in output_leaf:
> +                yield output_leaf
> +                continue
> +
> +            input_filename = '%s/%s.in' % (self._top_source_directory,

os.path.join(self._top_source_directory, '%s.in' % output_leaf)

@@ +61,5 @@
> +                continue
> +
> +            input_filename = '%s/%s.in' % (self._top_source_directory,
> +                output_leaf)
> +            output_filename = '%s/%s' % (output_directory, output_leaf)

likewise

@@ +64,5 @@
> +                output_leaf)
> +            output_filename = '%s/%s' % (output_directory, output_leaf)
> +
> +            output_directory_leaf = os.path.dirname(output_leaf)
> +            output_directory_full = '%s/%s' % (output_directory,

likewise

@@ +96,5 @@
> +
> +            def on_missing(variable):
> +                have_missing = True
> +
> +            makefile.perform_substitutions(mapping, fn_on_missing=on_missing)

You should just use Preprocessor here, and skip the Makefile abstraction altogether.
Attachment #642326 - Flags: review?(mh+mozilla) → review-
(Reporter)

Comment 11

5 years ago
(In reply to Mike Hommey [:glandium] from comment #10)
> Comment on attachment 642326 [details] [diff] [review]
> Implement make file variable substitution in Python
> 
> Review of attachment 642326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/pylib/mozbuild/mozbuild/makefile/conversion.py
> @@ +1,5 @@
> > +# 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/.
> > +
> > +import os.path
> 
> Since you import os, you don't need to import os.path.

Really?! I had no clue it did that! Cool!

> 
> @@ +7,5 @@
> > +
> > +from mozbuild.makefile.makefile import Makefile
> > +
> > +class MakefileSubstitutor(object):
> > +    """Helps performs variable substitutions in Makefile's.
> 
> Since I'd like to make this the base for a config.status replacement, you
> might as well not make it Makefile specific (we also AC_OUTPUT non makefiles)

Can do!

> 
> @@ +55,5 @@
> > +            'top_srcdir': self._top_source_directory,
> > +        }
> > +
> > +        for output_leaf in output_leafs:
> > +            if not output_leaf.endswith('Makefile') or ':' in output_leaf:
> 
> os.path.basename(output_leaf) == 'Makefile'
> 
> Why the ':' in output_leaf test? I'd expect this to skip everything on
> windows

I copied the behavior of acoutput-fast.pl:103. If that file was documented better, I could tell you why it is that way. I copied the behavior without asking questions because I wanted this to be a fully-compatible replacement. If you instruct me otherwise, I'll gladly change it.

> 
> @@ +59,5 @@
> > +            if not output_leaf.endswith('Makefile') or ':' in output_leaf:
> > +                yield output_leaf
> > +                continue
> > +
> > +            input_filename = '%s/%s.in' % (self._top_source_directory,
> 
> os.path.join(self._top_source_directory, '%s.in' % output_leaf)

Will this have implications for pymake in terms of expected slash? Maybe that's why the ':' exception exists in aclocal-fast?

> @@ +96,5 @@
> > +
> > +            def on_missing(variable):
> > +                have_missing = True
> > +
> > +            makefile.perform_substitutions(mapping, fn_on_missing=on_missing)
> 
> You should just use Preprocessor here, and skip the Makefile abstraction
> altogether.

You mean the thing in build/Preprocessor.py? Or, do you mean the generic preprocessor I will be building (formerly perform_substitutions())?

FWIW, I have more plans for the Makefile class. But, if substitution logic is going to live elsewhere, as a more generic thing, I guess that means there is no need to introduce this class yet.
(Assignee)

Comment 12

5 years ago
(In reply to Gregory Szorc [:gps] from comment #11)
> I copied the behavior of acoutput-fast.pl:103. If that file was documented
> better, I could tell you why it is that way. I copied the behavior without
> asking questions because I wanted this to be a fully-compatible replacement.
> If you instruct me otherwise, I'll gladly change it.

Mmmm then it /might/ be to eliminate the fileA:fileB syntax for CONFIG_FILES, but I think it's something that autoconf 2.13 doesn't support anyway.

> > os.path.join(self._top_source_directory, '%s.in' % output_leaf)
> 
> Will this have implications for pymake in terms of expected slash? Maybe
> that's why the ':' exception exists in aclocal-fast?

Good point. I don't know. Better try all possible combinations and see what happens. The sad thing is that the end result, after bug 742795, won't have to care about that because there won't be any unhandled file. So if we combine both efforts, you can skip figuring this out.

> > You should just use Preprocessor here, and skip the Makefile abstraction
> > altogether.
> 
> You mean the thing in build/Preprocessor.py?

Yes
(Assignee)

Updated

5 years ago
Assignee: gps → mh+mozilla
(Assignee)

Comment 13

5 years ago
Created attachment 644859 [details] [diff] [review]
PoC

This works quite nicely, but needs some more work to be a proper replacement for config.status (and a small refactor, to move some more stuff from config.status.m4 to ConfigStatus.py).
(Assignee)

Comment 14

5 years ago
Created attachment 644862 [details] [diff] [review]
PoC

Without the "Generated..." comments. They were only there for validation.
(Assignee)

Updated

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

Comment 15

5 years ago
Created attachment 644864 [details] [diff] [review]
Validation

This disables acoutput-fast.pl to ensure all files are treated equal, but still run it to have the directories created, because autoconf's config.status won't create them (for instance, it doesn't create memory because there is not memory/Makefile, and then fails to create memory/mozjemalloc/Makefile).
Then we run both autoconf's config.status and the new python one, making the python one fail when it tries to write files (which it doesn't do when it just wants to write the same content as the existing file)
(Assignee)

Comment 16

5 years ago
(In reply to Mike Hommey [:glandium] from comment #13)
> Created attachment 644859 [details] [diff] [review]
> PoC
> 
> This works quite nicely

Except it breaks mozilla-config.h and ACDEFINES.
(Assignee)

Updated

5 years ago
Depends on: 776968
(Assignee)

Comment 17

5 years ago
Created attachment 646108 [details] [diff] [review]
PoC

This is almost final. All it needs is comments and formal validation. Try was happy with it.
(Assignee)

Updated

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

Updated

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

Comment 18

5 years ago
Created attachment 646247 [details] [diff] [review]
part 1 - Avoid Preprocessor.py replacing undefined variables with the attemptSubstitution filter
Attachment #646247 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 19

5 years ago
Created attachment 646248 [details] [diff] [review]
part 2 - Allow to disable markers in Preprocessor.py
Attachment #646248 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 20

5 years ago
Created attachment 646249 [details] [diff] [review]
part 3 - Remove variables with no AC_SUBST in autoconf.mk.in
Attachment #646249 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 21

5 years ago
Created attachment 646250 [details] [diff] [review]
part 4 - Add check-sync-dirs exception for *.pyc under build/
Attachment #646250 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 22

5 years ago
Created attachment 646252 [details] [diff] [review]
part 5 - Use explicit _NON_GLOBAL_ACDEFINES, and rely less on pattern matching

This one is needed because the replacement system will just not export the defines in the list, instead of trying pattern matching.
Attachment #646252 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 23

5 years ago
Created attachment 646641 [details] [diff] [review]
part 2 - Allow to disable markers in Preprocessor.py

Fixed a small issue.
Attachment #646641 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #646248 - Attachment is obsolete: true
Attachment #646248 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 24

5 years ago
Created attachment 646642 [details] [diff] [review]
part 6 - Replace autoconf handling of config files and headers with our own

This is the main part, with lots of comments. I'm also going to attach the patch I've been using on try for validation.
Attachment #646642 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 25

5 years ago
Created attachment 646646 [details] [diff] [review]
Validation

This is the validation patch. What it does:
- Run configure once with the patch queue applied
- Run configure a second time with part 5 and 6 backed out
- Compare the resulting objdirs

The result is on try:
https://tbpl.mozilla.org/?tree=Try&rev=6e782aa0e144

What this shows is:
- Differences in a few files that are not generated by acoutput-fast.pl, and that contain extra "generated by configure" comments when the patch is backed out.
- Differences that come directly from changes in some Makefiles in part 6
- Whitespace differences on ACDEFINES (at the beginning and/or the end of the line)
- Weird gyp difference that has nothing to do with the patch.

All in all, this generates the same objdir for all platforms.
(Assignee)

Comment 26

5 years ago
Created attachment 646647 [details] [diff] [review]
part 7 - Remove make-makefile
Attachment #646647 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 27

5 years ago
Created attachment 646648 [details] [diff] [review]
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in

This was automatically changed with a script I'm going to attach. This uses a new automatic variable from part 6.
Attachment #646648 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

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

Updated

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

Updated

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

Comment 28

5 years ago
Created attachment 646651 [details]
Script used to generate the bonus patch
(Assignee)

Comment 29

5 years ago
Created attachment 646654 [details] [diff] [review]
part 7 - Remove make-makefile and acoutput-fast.pl

acoutput-fast.pl can die, too.
Attachment #646654 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #646647 - Attachment is obsolete: true
Attachment #646647 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Blocks: 778236
(Assignee)

Comment 30

5 years ago
Created attachment 646662 [details] [diff] [review]
part 6 - Replace autoconf handling of config files and headers with our own

Just added a little comment.
Attachment #646662 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #646642 - Attachment is obsolete: true
Attachment #646642 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 31

5 years ago
Created attachment 646677 [details] [diff] [review]
part 6 - Replace autoconf handling of config files and headers with our own

Fixed a bug in the relpath implementation for python <= 2.5, which I copied from testing/mozbase/manifestdestiny/manifestparser/manifestparser.py (which, thus, also has the bug).
Attachment #646677 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #646662 - Attachment is obsolete: true
Attachment #646662 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 32

5 years ago
(In reply to Mike Hommey [:glandium] from comment #31)
> Created attachment 646677 [details] [diff] [review]
> part 6 - Replace autoconf handling of config files and headers with our own
> 
> Fixed a bug in the relpath implementation for python <= 2.5, which I copied
> from testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
> (which, thus, also has the bug).

Since we are copying code, why not split it out into a shared module? The virtualenv is populated by the time all this code runs, right?
(Assignee)

Comment 33

5 years ago
(In reply to Gregory Szorc [:gps] from comment #32)
> (In reply to Mike Hommey [:glandium] from comment #31)
> > Created attachment 646677 [details] [diff] [review]
> > part 6 - Replace autoconf handling of config files and headers with our own
> > 
> > Fixed a bug in the relpath implementation for python <= 2.5, which I copied
> > from testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
> > (which, thus, also has the bug).
> 
> Since we are copying code, why not split it out into a shared module? The
> virtualenv is populated by the time all this code runs, right?

There is no virtualenv in standalone js. And we'll be able to remote it anyways when we switch to requring python 2.6.
Comment on attachment 646648 [details] [diff] [review]
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in

Just to ask/pose-the-thought, explicitly, will this change relative paths (and depth) on us for comm-central builds?
(Assignee)

Comment 35

5 years ago
(In reply to Justin Wood (:Callek) from comment #34)
> Comment on attachment 646648 [details] [diff] [review]
> bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in
> 
> Just to ask/pose-the-thought, explicitly, will this change relative paths
> (and depth) on us for comm-central builds?

The script used to do the conversion made sure the values before and after were the same.
(Assignee)

Updated

5 years ago
Blocks: 742795
(Assignee)

Comment 36

5 years ago
Created attachment 646828 [details] [diff] [review]
part 6 - Replace autoconf handling of config files and headers with our own

Last few changes for a green unittest on windows.
Attachment #646828 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Attachment #646677 - Attachment is obsolete: true
Attachment #646677 - Flags: review?(ted.mielczarek)
Blocks: 593585
Attachment #646247 - Flags: review?(ted.mielczarek) → review+
Attachment #646249 - Flags: review?(ted.mielczarek) → review+
Attachment #646250 - Flags: review?(ted.mielczarek) → review+
Attachment #646252 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 646641 [details] [diff] [review]
part 2 - Allow to disable markers in Preprocessor.py

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

::: config/Preprocessor.py
@@ +90,5 @@
>    
>    def setMarker(self, aMarker):
>      """
>      Set the marker to be used for processing directives.
>      Used for handling CSS files, with pp.setMarker('%'), for example.

Can you note in the comment that None is an acceptable value?

::: config/tests/unit-Preprocessor.py
@@ +42,5 @@
> +    self.pp.do_include(f)
> +    self.assertEqual(self.pp.out.getvalue(), """#if 0
> +PASS
> +#endif
> +""")

I might stick the content in a variable so you don't have to repeat it verbatim.
Attachment #646641 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 646648 [details] [diff] [review]
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in

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

I spot-checked a few of these files and they look okay. I'm not going to look at every single one. Very nice, though!
Attachment #646648 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 646654 [details] [diff] [review]
part 7 - Remove make-makefile and acoutput-fast.pl

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

\o/ So much win!
Attachment #646654 - Flags: review?(ted.mielczarek) → review+
Attachment #646651 - Attachment mime type: text/x-python → text/plain
Comment on attachment 646828 [details] [diff] [review]
part 6 - Replace autoconf handling of config files and headers with our own

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

::: aclocal.m4
@@ +1,4 @@
>  dnl
>  dnl Local autoconf macros used with mozilla
>  dnl The contents of this file are under the Public Domain.
>  dnl 

Can you kill this trailing space while you're here?

::: build/ConfigStatus.py
@@ +22,5 @@
> +# We need relpath, but it is introduced in python 2.6
> +# http://docs.python.org/library/os.path.html
> +try:
> +    relpath = os.path.relpath
> +except AttributeError:

You could do this without the attribute error by just doing:
def my_relpath(...):
  ...

relpath = getattr(os.path, "relpath", my_relpath)

@@ +82,5 @@
> +        log("creating %s" % relpath(self.filename, os.curdir))
> +        ensureParentDir(self.filename)
> +        file = open(self.filename, 'w')
> +        file.write(buf)
> +        file.close()

with open(self.filename, 'w'):
   ...

@@ +97,5 @@
> +
> +class ConfigEnvironment(object):
> +    '''A ConfigEnvironment is defined by a source directory and a build
> +    directory. It preprocesses files from the source directory and stores
> +    the result in the build directory.

Conventionally we refer to the build directory as the "object directory" or objdir. Should we continue that here?

@@ +188,5 @@
> +        pp.do_filter('attemptSubstitution')
> +        pp.setMarker(None)
> +        pp.out = FileAvoidWrite(path)
> +        pp.do_include(input)
> +        pp.out.close()

Can you use FileAvoidWrite in a with statement here?

@@ +200,5 @@
> +            "#define NAME ORIGINAL_VALUE" is turned into "#define NAME VALUE"
> +            "#undef UNKNOWN_NAME" is turned into "/* #undef UNKNOWN_NAME */"
> +            Whitespaces are preserved.
> +        '''
> +        input = open(self.get_input(path), 'rU')

with open(...) as input:

@@ +217,5 @@
> +                            l = l[:m.start('value')] + str(self.defines[name]) + l[m.end('value'):]
> +                        elif cmd == 'undef':
> +                            l = l[:m.start('cmd')] + 'define' + l[m.end('cmd'):m.end('name')] + ' ' + str(self.defines[name]) + l[m.end('name'):]
> +                    elif cmd == 'undef':
> +                       l = '/* ' + l[:m.end('name')] + ' */' + l[m.end('name'):]

This is a bit hard to read, but the comment at the top of the function explains it well.

::: build/Makefile.in
@@ +265,5 @@
>  GARBAGE += $(srcdir)/automationutils.pyc
> +
> +# Test for ConfigStatus.py
> +check::
> +	$(PYTHON) $(srcdir)/tests/unit-ConfigStatus.py

We should probably add a PYUNIT rule to rules.mk.

::: build/autoconf/config.status.m4
@@ +2,5 @@
> +dnl License, v. 2.0. If a copy of the MPL was not distributed with this
> +dnl file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +dnl For use in AC_SUBST replacement
> +define([MOZ_DIVERSION_SUBST], 11)

Magic number FDs scare me a bit, but I guess we can be reasonably sure that nothing is going to stomp on this, right?

::: config/rules.mk
@@ +1171,5 @@
>  endif
>  
>  $(DEPTH)/config/autoconf.mk: $(topsrcdir)/config/autoconf.mk.in
> +	$(DEPTH)/config.status -n --file=$(DEPTH)/config/autoconf.mk
> +	touch $@

You could use $(TOUCH) here, since that's a Pymake native command. (It just doesn't support any options.)
Attachment #646828 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 41

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f1d6b6cd63
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc06248ee601
https://hg.mozilla.org/integration/mozilla-inbound/rev/4440a3d03b51
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1693a9b908
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4218536a9e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c3069ddc4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ac79f11192
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef51674316a
Target Milestone: --- → mozilla17
(Assignee)

Comment 42

5 years ago
Backed out the bonus. It turns out the script didn't work so well :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc17369c9a6
https://hg.mozilla.org/mozilla-central/rev/c3f1d6b6cd63
https://hg.mozilla.org/mozilla-central/rev/bc06248ee601
https://hg.mozilla.org/mozilla-central/rev/4440a3d03b51
https://hg.mozilla.org/mozilla-central/rev/ad1693a9b908
https://hg.mozilla.org/mozilla-central/rev/b4218536a9e6
https://hg.mozilla.org/mozilla-central/rev/21c3069ddc4a
https://hg.mozilla.org/mozilla-central/rev/75ac79f11192
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 646648 [details] [diff] [review]
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in

(In reply to Mike Hommey [:glandium] from comment #42)
> Backed out the bonus. It turns out the script didn't work so well :(
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc17369c9a6

Should this patch then be set as obsolete? (so as not to confuse with other landed patches)
(Assignee)

Comment 45

5 years ago
Reopening for the bonus patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 46

5 years ago
Created attachment 649019 [details] [diff] [review]
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in.

Refreshed with a new script I'm going to attach.
(Assignee)

Updated

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

Comment 47

5 years ago
Created attachment 649020 [details]
Script used to generate the bonus patch
Attachment #646651 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #649020 - Attachment mime type: text/x-python → text/plain
(Assignee)

Comment 48

5 years ago
Landed the fixed bonus patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/162130598df0
Depends on: 780414

Updated

5 years ago
Depends on: 780421
https://hg.mozilla.org/mozilla-central/rev/162130598df0
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 780430
For some reason after this patch landed (verified by updating to its parent), I can't autoconf213 and re-configure my spidermonkey shell builds (dbg or opt), even with rm -rf of the objdirs. Halp?

/be
I'm still on Snow Leopard till Monday, Python 2.6.6.

/be
Glandium pointed to bug 780414, duh -- also gave a workaround patch there (thanks).

/be
(Assignee)

Updated

5 years ago
Depends on: 780446
(Assignee)

Updated

5 years ago
Depends on: 780457

Updated

5 years ago
Blocks: 780485

Updated

5 years ago
Blocks: 780357
(Reporter)

Updated

5 years ago
Blocks: 781366

Comment 53

5 years ago
Sorry to spam this bug, but it looks like it is busting thunderbird compilation on python3 linux based distro. See bug 781525.
(Assignee)

Updated

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