Last Comment Bug 774032 - Implement autoconf substitution in Python
: Implement autoconf substitution in Python
Status: RESOLVED FIXED
[buildfaster:?]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 797508 776968 780414 780421 780430 780446 780457 785269
Blocks: machfeatures 593585 742795 778236 780357 780485 781366
  Show dependency treegraph
 
Reported: 2012-07-14 20:35 PDT by Gregory Szorc [:gps]
Modified: 2012-12-11 20:32 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement make file variable substitution in Python (17.49 KB, patch)
2012-07-14 20:35 PDT, Gregory Szorc [:gps]
mh+mozilla: review-
Details | Diff | Splinter Review
Replace acoutput-fast.pl in main configure (1.80 KB, patch)
2012-07-14 20:40 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
PoC (26.39 KB, patch)
2012-07-23 01:17 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
PoC (25.88 KB, patch)
2012-07-23 01:23 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Validation (11.57 KB, patch)
2012-07-23 01:27 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
PoC (58.39 KB, patch)
2012-07-26 05:49 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 1 - Avoid Preprocessor.py replacing undefined variables with the attemptSubstitution filter (2.82 KB, patch)
2012-07-26 12:03 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
part 2 - Allow to disable markers in Preprocessor.py (3.18 KB, patch)
2012-07-26 12:03 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 3 - Remove variables with no AC_SUBST in autoconf.mk.in (5.11 KB, patch)
2012-07-26 12:04 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
part 4 - Add check-sync-dirs exception for *.pyc under build/ (450 bytes, patch)
2012-07-26 12:04 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
part 5 - Use explicit _NON_GLOBAL_ACDEFINES, and rely less on pattern matching (2.71 KB, patch)
2012-07-26 12:06 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
part 2 - Allow to disable markers in Preprocessor.py (3.17 KB, patch)
2012-07-27 11:20 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
part 6 - Replace autoconf handling of config files and headers with our own (71.89 KB, patch)
2012-07-27 11:25 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Validation (78.06 KB, patch)
2012-07-27 11:31 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 7 - Remove make-makefile (105.31 KB, patch)
2012-07-27 11:34 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in (724.84 KB, patch)
2012-07-27 11:35 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Script used to generate the bonus patch (43.46 KB, text/plain)
2012-07-27 11:40 PDT, Mike Hommey [:glandium]
no flags Details
part 7 - Remove make-makefile and acoutput-fast.pl (114.87 KB, patch)
2012-07-27 11:42 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
part 6 - Replace autoconf handling of config files and headers with our own (72.32 KB, patch)
2012-07-27 12:03 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 6 - Replace autoconf handling of config files and headers with our own (72.33 KB, patch)
2012-07-27 12:54 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 6 - Replace autoconf handling of config files and headers with our own (72.37 KB, patch)
2012-07-28 02:19 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in. (792.88 KB, patch)
2012-08-04 11:28 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Script used to generate the bonus patch (2.15 KB, text/plain)
2012-08-04 11:30 PDT, Mike Hommey [:glandium]
no flags Details

Description Gregory Szorc [:gps] 2012-07-14 20:35:42 PDT
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.
Comment 1 Gregory Szorc [:gps] 2012-07-14 20:40:59 PDT
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.
Comment 2 Gregory Szorc [:gps] 2012-07-14 20:54:51 PDT
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).
Comment 3 Mike Hommey [:glandium] 2012-07-14 23:21:10 PDT
(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.
Comment 4 Gregory Szorc [:gps] 2012-07-14 23:43:20 PDT
(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?
Comment 5 Mike Hommey [:glandium] 2012-07-15 00:14:34 PDT
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.
Comment 6 Gregory Szorc [:gps] 2012-07-15 00:25:48 PDT
(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?
Comment 7 Mike Hommey [:glandium] 2012-07-15 00:40:13 PDT
(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).
Comment 8 Mike Hommey [:glandium] 2012-07-15 00:41:28 PDT
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.
Comment 9 Gregory Szorc [:gps] 2012-07-15 01:22:39 PDT
(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.
Comment 10 Mike Hommey [:glandium] 2012-07-16 08:15:58 PDT
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.
Comment 11 Gregory Szorc [:gps] 2012-07-16 17:56:59 PDT
(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.
Comment 12 Mike Hommey [:glandium] 2012-07-16 23:49:42 PDT
(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
Comment 13 Mike Hommey [:glandium] 2012-07-23 01:17:29 PDT
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).
Comment 14 Mike Hommey [:glandium] 2012-07-23 01:23:01 PDT
Created attachment 644862 [details] [diff] [review]
PoC

Without the "Generated..." comments. They were only there for validation.
Comment 15 Mike Hommey [:glandium] 2012-07-23 01:27:12 PDT
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)
Comment 16 Mike Hommey [:glandium] 2012-07-23 03:43:03 PDT
(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.
Comment 17 Mike Hommey [:glandium] 2012-07-26 05:49:10 PDT
Created attachment 646108 [details] [diff] [review]
PoC

This is almost final. All it needs is comments and formal validation. Try was happy with it.
Comment 18 Mike Hommey [:glandium] 2012-07-26 12:03:33 PDT
Created attachment 646247 [details] [diff] [review]
part 1 - Avoid Preprocessor.py replacing undefined variables with the attemptSubstitution filter
Comment 19 Mike Hommey [:glandium] 2012-07-26 12:03:51 PDT
Created attachment 646248 [details] [diff] [review]
part 2 - Allow to disable markers in Preprocessor.py
Comment 20 Mike Hommey [:glandium] 2012-07-26 12:04:13 PDT
Created attachment 646249 [details] [diff] [review]
part 3 - Remove variables with no AC_SUBST in autoconf.mk.in
Comment 21 Mike Hommey [:glandium] 2012-07-26 12:04:50 PDT
Created attachment 646250 [details] [diff] [review]
part 4 - Add check-sync-dirs exception for *.pyc under build/
Comment 22 Mike Hommey [:glandium] 2012-07-26 12:06:07 PDT
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.
Comment 23 Mike Hommey [:glandium] 2012-07-27 11:20:45 PDT
Created attachment 646641 [details] [diff] [review]
part 2 - Allow to disable markers in Preprocessor.py

Fixed a small issue.
Comment 24 Mike Hommey [:glandium] 2012-07-27 11:25:57 PDT
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.
Comment 25 Mike Hommey [:glandium] 2012-07-27 11:31:31 PDT
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.
Comment 26 Mike Hommey [:glandium] 2012-07-27 11:34:32 PDT
Created attachment 646647 [details] [diff] [review]
part 7 - Remove make-makefile
Comment 27 Mike Hommey [:glandium] 2012-07-27 11:35:42 PDT
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.
Comment 28 Mike Hommey [:glandium] 2012-07-27 11:40:11 PDT
Created attachment 646651 [details]
Script used to generate the bonus patch
Comment 29 Mike Hommey [:glandium] 2012-07-27 11:42:37 PDT
Created attachment 646654 [details] [diff] [review]
part 7 - Remove make-makefile and acoutput-fast.pl

acoutput-fast.pl can die, too.
Comment 30 Mike Hommey [:glandium] 2012-07-27 12:03:29 PDT
Created attachment 646662 [details] [diff] [review]
part 6 - Replace autoconf handling of config files and headers with our own

Just added a little comment.
Comment 31 Mike Hommey [:glandium] 2012-07-27 12:54:19 PDT
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).
Comment 32 Gregory Szorc [:gps] 2012-07-27 13:51:04 PDT
(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?
Comment 33 Mike Hommey [:glandium] 2012-07-27 14:22:58 PDT
(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 34 Justin Wood (:Callek) 2012-07-27 22:13:56 PDT
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?
Comment 35 Mike Hommey [:glandium] 2012-07-27 23:37:56 PDT
(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.
Comment 36 Mike Hommey [:glandium] 2012-07-28 02:19:57 PDT
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.
Comment 37 Ted Mielczarek [:ted.mielczarek] 2012-08-03 11:50:01 PDT
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.
Comment 38 Ted Mielczarek [:ted.mielczarek] 2012-08-03 11:50:52 PDT
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!
Comment 39 Ted Mielczarek [:ted.mielczarek] 2012-08-03 11:51:45 PDT
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!
Comment 40 Ted Mielczarek [:ted.mielczarek] 2012-08-03 13:14:59 PDT
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.)
Comment 42 Mike Hommey [:glandium] 2012-08-04 01:47:55 PDT
Backed out the bonus. It turns out the script didn't work so well :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc17369c9a6
Comment 44 Gary Kwong [:gkw] [:nth10sd] 2012-08-04 11:19:56 PDT
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)
Comment 45 Mike Hommey [:glandium] 2012-08-04 11:28:20 PDT
Reopening for the bonus patch.
Comment 46 Mike Hommey [:glandium] 2012-08-04 11:28:56 PDT
Created attachment 649019 [details] [diff] [review]
bonus - Use @DEPTH@ and @relativesrcdir@ in Makefile.in.

Refreshed with a new script I'm going to attach.
Comment 47 Mike Hommey [:glandium] 2012-08-04 11:30:16 PDT
Created attachment 649020 [details]
Script used to generate the bonus patch
Comment 48 Mike Hommey [:glandium] 2012-08-04 11:33:57 PDT
Landed the fixed bonus patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/162130598df0
Comment 49 Ryan VanderMeulen [:RyanVM] 2012-08-04 18:42:37 PDT
https://hg.mozilla.org/mozilla-central/rev/162130598df0
Comment 50 Brendan Eich [:brendan] 2012-08-04 22:08:52 PDT
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
Comment 51 Brendan Eich [:brendan] 2012-08-04 22:09:24 PDT
I'm still on Snow Leopard till Monday, Python 2.6.6.

/be
Comment 52 Brendan Eich [:brendan] 2012-08-04 23:22:54 PDT
Glandium pointed to bug 780414, duh -- also gave a workaround patch there (thanks).

/be
Comment 53 Frederic Bezies 2012-08-09 08:45:40 PDT
Sorry to spam this bug, but it looks like it is busting thunderbird compilation on python3 linux based distro. See bug 781525.

Note You need to log in before you can comment on or make changes to this bug.