Last Comment Bug 687511 - build/autoconf/make-makefile enhancements
: build/autoconf/make-makefile enhancements
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on: 713248
Blocks: 687515
  Show dependency treegraph
 
Reported: 2011-09-19 09:57 PDT by Joey Armstrong [:joey]
Modified: 2011-12-23 09:10 PST (History)
7 users (show)
emorley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
edits allowing make-makefile to be used from the sandbox root, unit test setup. (101.41 KB, patch)
2011-09-21 13:40 PDT, Joey Armstrong [:joey]
coop: feedback+
Details | Diff | Splinter Review
edits allowing make-makefile to be used from the sandbox root, unit test setup. (144.50 KB, patch)
2011-10-05 12:38 PDT, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Splinter Review
edits for using make-makefile from sandbox root & unit tests added. (120.87 KB, patch)
2011-10-24 07:21 PDT, Joey Armstrong [:joey]
coop: review+
coop: checkin-
Details | Diff | Splinter Review
original make-makefile enhancements + fix for pymake (121.92 KB, patch)
2011-11-10 10:30 PST, Joey Armstrong [:joey]
khuey: review+
coop: review+
Details | Diff | Splinter Review
patch diff (2.36 KB, text/plain)
2011-11-10 11:49 PST, Joey Armstrong [:joey]
no flags Details
original make-makefile enhancements + fix for pymake (121.93 KB, patch)
2011-11-19 04:04 PST, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2011-09-19 09:57:19 PDT
make-makefile is not playing nicely with the container makefiles when invoked beneath js/src/config.  Add new options to allow invoking the command from root of the tree.  -t $top exists, add option --obj to explicitly specify $MOZ_OBJDIR.

The presence of both arguments will allow handling Makefile.in and generated Makefile paths as a relative path which will simplify parsing.  Also DEPTH= can be calculated directly from the relative path when DEPTH= cannot be found embedded within Makefile.in.

Also:
1) Setup unit tests to lock down existing behavior.
2) Move several make-makefile functions into a module so they can be tested w/o shell overhead from invoking make-makefile.
2) Replace inlined code with std library functions -- File::Path::mkdir(), File::Basename::dirname(), File::Copy::move() etc.
   - Cwd and dirname() could be used to replace `pwd -W` and sub dirname() logic but verify and add unit tests for MinGW paths before going live with changes.
3) Use std Getopts module for command line argument parsing.
4) Exit status is not being checked during the "mkdir -p" and run_config_status { system("config.status") } calls.  Missing config.status files is questionable but directory creation failures should be detected and reported.
5) Replace "mkdir -p" with File::Path::mkdir() and a recursive helper subroutine.  mkdir() is a perl builtin and recursive traversal will have less overhead than spawning a shell.  If an early script version was written as shell("mkdir $dir") that might explain where system("mkdir -p @dir_list) came from.
6)Preserve timestamps when generating makefiles.  Only over-write Makefile when contents have changed.
7) make-makefile should exit with a non-zero exit status when problems are detected.
Comment 1 Joey Armstrong [:joey] 2011-09-21 13:40:47 PDT
Created attachment 561557 [details] [diff] [review]
edits allowing make-makefile to be used from the sandbox root, unit test setup.

See bug descr for an itemized list of changes.

Added option --obj ${MOZ_OBJDIR}.  When --topsrc and --obj are passed drive a relative path to Makefile/Makefile.in and search for files as needed.  Existing logic is unchanged, container makefiles can utilize the new logic during makefile generation.


Script will now exit with a non-zero status when errors are detected.

shell("mkdir -p") replaced with File::Path::mkpath() and a recursive helper routine mkdirr().  Directory creation failures will propogate back to the caller rather than being silenty ignored { shell exit status was not being checked }.

run_config_status() will also return error when config.status is mia.

Functions moved out of make-makefile and into make_makefile.pm to support unit testing.  Tests added to verify getRelPath(), handling of argument permutations and makefile generation.  getDepth(), getRelPath(), etc are all backed by testing subroutines.

some global vars are in play.  perl is older than 5.10 for a few platforms so "use feature 'state';" cannot be used for static vars within function scope.

Std modules were used to replace inlined logic.  Getopt, File::Basename, File::path, etc.

Reworked some of the parsing/string replacement logic used to fill in a makefile template to be more efficient.  Recursive inline replacement of @token@ strings.

Preserve makefile timestamps -- compare generated against existing and only over-write when makefile does not exist or content has changed.

build/autoconf/test/Makefile.in added, check target will invoke the unit test, dependencies only allow test to run when sources change.

build/autoconf edits copied beneath js/src/build so check_sync-dirs.py will not complain.
Comment 2 Aki Sasaki [:aki] 2011-09-21 18:29:09 PDT
Comment on attachment 561557 [details] [diff] [review]
edits allowing make-makefile to be used from the sandbox root, unit test setup.

>-# 1.1 (the "License"); you may not use this file except in compliance with
>+# 1.1 (the "License"); you may not use his file except in compliance with

Did you mean to s,this,his, ? (Going to guess this is a typo.)

>+use strict;
>+use warnings;

Joey++
Also for all the 'my's instead of globals.

>+  my $depth = $argv{depth} || '';
>+  if (! $depth)
>+  {
>+      foreach my $fyl (@args)
>+      {
>+    if (my $tmp = find_depth($fyl))
>+    {
>+        $depth = $tmp;
>+        last;
>+    }
>+      }
>+  }

Is this really the indentation?
Looks like tab/space mixing? (not sure if it's bugzilla patch munging or
what; I saw it all over the patch)

>   # Build the list of makefiles to generate
>   #
>   @makefiles = ();
>   my $makefile;
>-  foreach $makefile (@args) {
>-    $makefile =~ s/\.in$//;
>-    $makefile =~ s/\/$//;
>-    $makefile =~ /Makefile$/
>+  while (@args)
>+  {
>+      next unless my $makefile = shift @args;
>+      $makefile =~ s/\.in$//;
>+      $makefile =~ s/\/$//;
>+      $makefile =~ /Makefile$/

Looks like we have two levels of 'my $makefile' here -- probably remove the my $makefile; above the |while (@args)| ?

>+###########################################################################
>+## Intent: Read in configure values and return a hash of key/value pairs
>+## -----------------------------------------------------------------------
>+## Args:
>+##   none
>+## Returns:
>+##   hash  configure data to use for makefile substitutions
>+## -----------------------------------------------------------------------
>+###########################################################################

Have you thought about putting these comments in POD format for Perldoc?
https://secure.wikimedia.org/wikipedia/en/wiki/Perldoc

I haven't really gone into depth at all on the pm's functions; I can try if you want me to.
Comment 3 Gregory Szorc [:gps] 2011-09-21 18:42:15 PDT
Could you also 'use English' and get rid of the 2 character scalars? It increases readability dramatically (especially for the non-Perl people). http://perldoc.perl.org/English.html. As for other nits, please don't use the magic $_. There are so many ways to accidentally break things using it.

I also recommend running Perl through Perl::Critic. If you don't have that installed locally, go to http://perlcritic.com/. Use the brutal profile if you can (with minor exceptions for things you disagree with). It is annoying at first, but once you get the hang of it, it dramatically increases the readability of Perl code and decreases bugs.
Comment 4 Joey Armstrong [:joey] 2011-09-22 06:26:11 PDT
(In reply to Gregory Szorc [:gps] from comment #3)
> Could you also 'use English' and get rid of the 2 character scalars? It
> increases readability dramatically (especially for the non-Perl people).
> http://perldoc.perl.org/English.html.

There is a bit of a performance hit with using the English module.  All of the 2 character magic variables need to be aliased-into/de-referenced-from the global symbol table.

Also there were plans to deprecated the global symbol table with a newer interpreter release { $_, $#, etc will be going away } so not sure how these dependencies will play out for the module.

If English is no longer able to simply alias symbol table variables { to avoid breaking existing functionality} it may need to either allocate static storage for them (with "use feature 'state'") or somehow mirror the active symbol table.  That seems like a lot of overhead for the module to contend with going forward.  If the magic variables are deprecated I would not be surprised if English.pm were as well.

> As for other nits, please don't use
> the magic $_. There are so many ways to accidentally break things using it.

fyi>  Adding 'local $_;' within subroutines is a guard against these types of problems.  Variables are duplicated within perl's symbol table for the subroutine call so edits made within the subroutine cannot bleed back into the caller.

Most of the loops are using my $var variables to local storage but there are a few places use of $_ is preferable.  Within I/O loops localized variables will require allocating storage for and copying the value.  Not a big deal for small files but this can add up for CDs or files in th GB range.

Also when writing parsers with a large number of regexprs, not using local variables can save a good deal of extra typing.  I find the 2nd syntax a lot less cluttered than the first:
   if ($var =~ /foobar/)
   elsif ($var =~ /tansfans/)
   ...
-vs-
    if (/foobar/)
     elsif (/tansfans/)

 
> I also recommend running Perl through Perl::Critic. If you don't have that
> installed locally, go to http://perlcritic.com/. Use the brutal profile if
> you can (with minor exceptions for things you disagree with). It is annoying
> at first, but once you get the hang of it, it dramatically increases the
> readability of Perl code and decreases bugs.

Writing unit tests to verify code behavior is by far the best way to flush out bugs and variant behavior :).  Unit tests are included in this patch for most of the functionality, both new and legacy.

Running scripts through perl -c, strict.pm and warnings.pm will flush out the vast majority of real problems.  I'll try out the perl-lint program to see what it is able to detect but systems with >100k lines of code were very well covered by strict, warnings & unit tests.

Thanks -- Joey
Comment 5 Joey Armstrong [:joey] 2011-09-22 06:37:03 PDT
(In reply to Aki Sasaki [:aki] from comment #2)
> Comment on attachment 561557 [details] [diff] [review]
> edits allowing make-makefile to be used from the sandbox root, unit test
> setup.
> 
> >-# 1.1 (the "License"); you may not use this file except in compliance with
> >+# 1.1 (the "License"); you may not use his file except in compliance with
> 
> Did you mean to s,this,his, ? (Going to guess this is a typo.)

Stray edit to be fixed.

> >+use strict;
> >+use warnings;
> 
> Joey++
> Also for all the 'my's instead of globals.

Yay for local storage, this also fixed a few uninit var problems.

 
> >+  my $depth = $argv{depth} || '';
> >+  if (! $depth)
> >+  {
> >+      foreach my $fyl (@args)
> >+      {
> >+    if (my $tmp = find_depth($fyl))
> >+    {
> >+        $depth = $tmp;
> >+        last;
> >+    }
> >+      }
> >+  }
> 
> Is this really the indentation?
> Looks like tab/space mixing? (not sure if it's bugzilla patch munging or
> what; I saw it all over the patch)

I'll check.  emacs is setup for proper whitespace indention but my vi session may not be.


> >   # Build the list of makefiles to generate
> >   #
> >   @makefiles = ();
> >   my $makefile;
> >-  foreach $makefile (@args) {
> >-    $makefile =~ s/\.in$//;
> >-    $makefile =~ s/\/$//;
> >-    $makefile =~ /Makefile$/
> >+  while (@args)
> >+  {
> >+      next unless my $makefile = shift @args;
> >+      $makefile =~ s/\.in$//;
> >+      $makefile =~ s/\/$//;
> >+      $makefile =~ /Makefile$/
> 
> Looks like we have two levels of 'my $makefile' here -- probably remove the
> my $makefile; above the |while (@args)| ?

Yes, my outside the loop was an early fix for 'use strict';  No longer needed since the my variable localized it to the loop.  Hmmm, surprised perl -c did not report this as a single use variable...



> >+###########################################################################
> >+## Intent: Read in configure values and return a hash of key/value pairs
> >+## -----------------------------------------------------------------------
> >+## Args:
> >+##   none
> >+## Returns:
> >+##   hash  configure data to use for makefile substitutions
> >+## -----------------------------------------------------------------------
> >+###########################################################################
> 
> Have you thought about putting these comments in POD format for Perldoc?
> https://secure.wikimedia.org/wikipedia/en/wiki/Perldoc

Yes could do that.  Most of the existing scripts are light on comments/documentation and none are decorated for perldoc use so this may be setting precedent.


> I haven't really gone into depth at all on the pm's functions; I can try if
> you want me to.

Whatever you feel like reviewing, thanks
Comment 6 Chris Cooper [:coop] [away until Aug 29] 2011-09-28 10:12:08 PDT
Comment on attachment 561557 [details] [diff] [review]
edits allowing make-makefile to be used from the sandbox root, unit test setup.

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

I stuck to reviewing the perl modules since Aki focused on the rest. Nothing major wrong here, just some nits interspersed.

I'm assuming that the version of make_makefiles.pm under js/src/build/autoconf is the same as the one under build/autoconf. My comments on the build/autoconf version apply to both.

Do we need to copy the unittest perl module under the js tree too?

::: build/autoconf/make_makefile.pm
@@ +104,5 @@
> +    {
> +	@data = <FYL>;
> +	close(FYL);
> +    }
> +    @data;

I prefer explicitly calling "return X" to make things clearer. This applies to all your subs.

@@ +146,5 @@
> +    $fargs{recursive} = 1;
> +
> +    my @errors;
> +    push(@errors, $@) if ($@);
> +    foreach (@_)

Better to assign this to a named variable for legibility, or at least provide a comment for non-perl devs as to where this comes from.

@@ +195,5 @@
> +sub getDepth($)
> +{
> +    my $fyl = shift || '';
> +
> +    my @path = split(m%/%o, $fyl);

Not a huge fan of the chosen delimiter here since it looks like string formatting at a glance.

@@ +250,5 @@
> +    my $name = basename($path0);
> +    my $haveMF = ($name eq 'Makefile.in') ? 1
> +	: ($name eq 'Makefile') ? -1
> +	: 0
> +	;

Ugh, wish perl had a switch statement.

Would it be better to have separate vars for tracking Makefile vs. Makefile.in? I'm not sure how I feel about using a three-state var here instead. Maybe the mapping just needs to be explicitly called out in a comment somewhere.

@@ +322,5 @@
> +    #######################################################################
> +    ## Generated makefile does not yet exist or path is invalid.
> +    ## Should this conditon be handled to detect non-existent Makefile.in:
> +    ##   Makefile.am => Makefile.in => Makefile but Makefile.in
> +    #######################################################################

Have you seen instances of this in the code?

@@ +407,5 @@
> +## Returns:
> +##   scalar - absolute path to the sandbox object directory
> +## -----------------------------------------------------------------------
> +###########################################################################
> +my $god_dir;

Great var name. ;)

@@ +445,5 @@
> +    }
> +
> +    $god_dir;
> +} # getObjDir
> +

Just hope you realize how hard it was for me to refrain from making undefined $god_dir and absolute_path jokes in this function. ;)

@@ +538,5 @@
> +} # updateMakefiles
> +
> +# Output the makefiles.
> +#
> +sub update_makefiles_legacy {

No header here like the other subs?

@@ +541,5 @@
> +#
> +sub update_makefiles_legacy {
> +  my ($ac_given_srcdir, $pac_given_srcdir, @makefiles) = @_;
> +  my $debug = $main::argv{debug} || 0;
> +  my $pwdcmd = ($^O eq 'msys') ? 'pwd -W' : 'pwd';

I'm totally going to steal that if I need to write more cross-platform perl.

::: build/autoconf/test/make_makefile.tpm
@@ +7,5 @@
> +##---] CORE/CPAN INCLUDES [---##
> +##----------------------------##
> +use strict;
> +use warnings;
> +use feature 'state';

You mentioned this wasn't available on Windows. Does this mean we can only unittest on the other platforms until we upgrade perl?

@@ +29,5 @@
> +
> +##------------------##
> +##---] INCLUDES [---##
> +##------------------##
> +use FindBin;

FindBin is included twice.

@@ +216,5 @@
> +	 'profile/dirserviceprovider/public'                               => 'profile/dirserviceprovider/public',
> +
> +	 # cwd + cleanup
> +	 # '../../../profile/dirserviceprovider/public/Makefile.in'          => 'profile/dirserviceprovider/public',
> +#	 "../../../${obj0}/profile/dirserviceprovider/public/Makefile.in"  => 'profile/dirserviceprovider/public',

Commented out on purpose?

@@ +222,5 @@
> +	 ## Special case: This could be handled but permutations of non-existent files, non-overlapping paths
> +	 ## and relative paths containing partial subdirectories will compilicate the logic.  Wait until needed.
> +         ## Relative path: $root + obj + subdir
> +#	 "${obj0}/profile/dirserviceprovider/public/Makefile"          => 'profile/dirserviceprovider/public',
> +	 join('/', $obj, 'profile/dirserviceprovider/public/Makefile') => 'profile/dirserviceprovider/public',

Do you still need the commented-out line here?

@@ +243,5 @@
> +    local $main::argv{obj} = $obj;
> +
> +    %data =
> +	(
> +#	 "$top/profile/dirserviceprovider/public/Makefile.in" => 'profile/dirserviceprovider/public',

Commented out on purpose?

@@ +288,5 @@
> +    ok(scalar @errors, 0, "Errors detected: @errors");
> +} # check_mkdirr
> +
> +###########################################################################
> +## Intent:

Still in progress, I guess?

@@ +304,5 @@
> +
> +} # check_run_config_status
> +
> +###########################################################################
> +## Intent:

Same here.

@@ +444,5 @@
> +    exit 1;
> +}
> +
> +init();
> +#testbyname(@{ $argv{test} }) if ($argv{test});

Still to be implemented?
Comment 7 Joey Armstrong [:joey] 2011-09-28 13:22:23 PDT
(In reply to Chris Cooper [:coop] from comment #6)
> Comment on attachment 561557 [details] [diff] [review] [diff] [details] [review]
> edits allowing make-makefile to be used from the sandbox root, unit test
> setup.
> 
> Review of attachment 561557 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I'm assuming that the version of make_makefiles.pm under
> js/src/build/autoconf is the same as the one under build/autoconf. My
> comments on the build/autoconf version apply to both.
> 
> Do we need to copy the unittest perl module under the js tree too?

The test directory was not copied beneath js/src to avoid having the test suite run twice { more shells for windows to spawn }.  build/unix/test/ has already been setup this way.


> ::: build/autoconf/make_makefile.pm
> @@ +104,5 @@
> > +    {
> > +	@data = <FYL>;
> > +	close(FYL);
> > +    }
> > +    @data;
> 
> I prefer explicitly calling "return X" to make things clearer. This applies
> to all your subs.

Should not be a problem with these functions but explicitly including return as the last function statement will double the amount of memory used.  Perl automagically allocates stack space for return values.  Mentioning 'return' will trigger allocating space for a 2nd set of values.


> @@ +250,5 @@
> > +    my $name = basename($path0);
> > +    my $haveMF = ($name eq 'Makefile.in') ? 1
> > +	: ($name eq 'Makefile') ? -1
> > +	: 0
> > +	;
> 
> Ugh, wish perl had a switch statement.
> 
> Would it be better to have separate vars for tracking Makefile vs.
> Makefile.in? I'm not sure how I feel about using a three-state var here
> instead. Maybe the mapping just needs to be explicitly called out in a
> comment somewhere.

I can add in more comments.  The logic is basically setting up to have a single code path through the function for finding and deriving path elements.  If separate vars were used more conditionals/logic will be needed for state & handling which may contribute to variant behavior in the branches.


> > +    #######################################################################
> > +    ## Generated makefile does not yet exist or path is invalid.
> > +    ## Should this conditon be handled to detect non-existent Makefile.in:
> > +    ##   Makefile.am => Makefile.in => Makefile but Makefile.in
> > +    #######################################################################
> 
> Have you seen instances of this in the code?

Makefile.am exists in the sandbox but not yet sure if the transitive derivation will be an issue.  No cases found yet for the script to work with just noting the possibility.


> @@ +407,5 @@
> > +## Returns:
> > +##   scalar - absolute path to the sandbox object directory
> > +## -----------------------------------------------------------------------
> > +###########################################################################
> > +my $god_dir;
> 
> Great var name. ;)
> 
> Just hope you realize how hard it was for me to refrain from making
> undefined $god_dir and absolute_path jokes in this function. ;)

That's pretty goo?d :)


> @@ +538,5 @@
> > +} # updateMakefiles
> > +
> > +# Output the makefiles.
> > +#
> > +sub update_makefiles_legacy {
> 
> No header here like the other subs?

The original function comment was transferred verbatim into the module.


> > +use strict;
> > +use warnings;
> > +use feature 'state';
> 
> You mentioned this wasn't available on Windows. Does this mean we can only
> unittest on the other platforms until we upgrade perl?

This patch submission predates the perl 5.10 upgrade bug.  Yes this is some of the 'cleanup' still needed to workaround the old interpreter/module problem.  The unit test will be usable on windows if the handful of state variables are replaced by package globals.


> @@ +216,5 @@
> > +	 # cwd + cleanup
> > +	 # '../../../profile/dirserviceprovider/public/Makefile.in'          => 'profile/dirserviceprovider/public',
> > +#	 "../../../${obj0}/profile/dirserviceprovider/public/Makefile.in"  => 'profile/dirserviceprovider/public',
> 
> Commented out on purpose?

Yep -- all of the unit tests paths commented out were added as permutations for coverage but they are not cases that make-makefile is able to handle properly.


> @@ +304,5 @@
> > +
> > +} # check_run_config_status
> > +
> > +###########################################################################
> > +## Intent:
> 
> Same here.

Function was undocumented in the original script.  The function was moved into the module for unit testing and a stub function header was added.  Yet to be documented.
Comment 8 Joey Armstrong [:joey] 2011-10-05 12:38:42 PDT
Created attachment 564961 [details] [diff] [review]
edits allowing make-makefile to be used from the sandbox root, unit test setup.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8b186be93783

Bug list:
     linuxqt - 673017 - segv in test_IHistory.cpp
linux mobile - 634051 - dpkg-architecture, command not found 
       win32 -        - (configure) cl : Command line error D8003 : missing source filename

Most of the functionality itemized in the first bug comment:
  o nit cleanups from Aki & Coop's code reviews.
  o Add option --obj allowing make-makefile to be invoked outside of MOZ_OBJDIR.
    Makefiles generated by recursive make calls should not be affected by this patch.  All makefiles in a clean sandbox compared with patch generated makefiles { topsrcdir=@@ paths normalized for comparison }.  Only deltas detected were for the unit test makefile added by this patch.
  o Replaced inlined logic with std perl modules (Cwd, Getopts, etc)
  o Added use strict; use warnings; and fixed fallout/uninit vars.
  o Moved several functions into module makemakefile.pm for testing.
  o Unit tests setup for both legacy behavior and new functionality.
  o A handful of state variables were replaced by package variables
    so unit tests can be run on a windows box.
  o Inline logic for determining DEPTH= value or directory paths
    migrated into named functions so they can be tested.
  o Preserve Makefile timestamps and exit with status.
  o Incorporate exit status from mkdir -p and config.status calls
  o copied build/autoconf edits beneath js/src/build to keep syncdirs happy.
  o added option --bench to report elapsed runtime.
  o perldoc comments added to document arguments
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-07 07:22:51 PDT
This is pretty large, probably not going to get to the review until next week.
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-10 05:09:30 PDT
Comment on attachment 564961 [details] [diff] [review]
edits allowing make-makefile to be used from the sandbox root, unit test setup.

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

I would give an r+, but I had a lot of questions, so I'm leaving the review pending until those are answered.

I didn't review the test code, and Coop went through the actual perl code in great detail so I mostly skimmed that.

::: build/autoconf/Makefile.in
@@ +45,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +ifdef ENABLE_TESTS
> +  DIRS += test

Rather than have this entire file, why not do

ifdef ENABLE_TESTS
  DIRS+=autoconf/tests
endif

in the parent directory?

::: build/autoconf/make-makefile
@@ +15,5 @@
>  # The Original Code is mozilla.org code.
>  #
>  # The Initial Developer of the Original Code is
>  # Netscape Communications Corporation.
> +# Portions created by the Initial Developer are Copyright (C) 1999-2011

There's no need to change the date here.  These don't actually mean anything (see bug 536336 for some fun legal reading).

::: build/autoconf/make-makefile.excl
@@ +1,5 @@
> +###########################################################################
> +## Intent: Exclusion list for container make builds
> +###########################################################################
> +
> +# EOF

Why is this here?

::: build/autoconf/test/Makefile.in
@@ +51,5 @@
> +ifneq (,$(findstring check,$(MAKECMDGOALS)))
> +          allsrc = $(wildcard $(srcdir)/*)
> +       tests2run = $(notdir $(filter %.tpl,$(allsrc)))
> +       tests2run += $(notdir $(filter %.tpm,$(allsrc)))
> +  check_targets += $(addprefix $(TS)/,$(tests2run))

Consistent indentation please.

@@ +68,5 @@
> +
> +parent = $(patsubst %/,%,$(dir $(srcdir)))
> +$(TS)/make-makefile.tpl: \
> +  $(srcdir)/make-makefile.tpl\
> +  $(parent)/makemakefile.pm\

Assuming you're trying to stay under 80 characters on this line, we generally just live with overlong lines in our Makefiles.

@@ +76,5 @@
> +
> +$(TS)/makemakefile.tpm: \
> +  $(srcdir)/makemakefile.tpm \
> +  $(parent)/makemakefile.pm \
> +  $(NULL)

Same.

@@ +78,5 @@
> +  $(srcdir)/makemakefile.tpm \
> +  $(parent)/makemakefile.pm \
> +  $(NULL)
> +	$(PERL) $(srcdir)/runtest $<
> +	@touch $@

Why do you have the wildcard target and then specific targets for .tpl/.tmp?  Can't we just make the wildcard target depend on $(parent)/makemakefile.pm and be done?

::: build/autoconf/test/make-makefile.excl
@@ +4,5 @@
> +
> +/dev/null
> +/foo/bar
> +/a/b/c
> +/a/b/d

And why is this here?

::: js/src/build/autoconf/make-makefile.excl
@@ +1,5 @@
> +###########################################################################
> +## Intent: Exclusion list for container make builds
> +###########################################################################
> +
> +# EOF

Same question.
Comment 11 Joey Armstrong [:joey] 2011-10-10 08:02:26 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Comment on attachment 564961 [details] [diff] [review] [diff] [details] [review]
> edits allowing make-makefile to be used from the sandbox root, unit test
> setup.
> 
> Review of attachment 564961 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I would give an r+, but I had a lot of questions, so I'm leaving the review
> pending until those are answered.
> 
> I didn't review the test code, and Coop went through the actual perl code in
> great detail so I mostly skimmed that.

Thanks

 
> ::: build/autoconf/Makefile.in
> @@ +45,5 @@
> > +
> > +include $(DEPTH)/config/autoconf.mk
> > +
> > +ifdef ENABLE_TESTS
> > +  DIRS += test
> 
> Rather than have this entire file, why not do
> 
> ifdef ENABLE_TESTS
>   DIRS+=autoconf/tests
> endif
> 
> in the parent directory?

The call could be moved not a big problem, more tests will be added
beneath build/ at some point which should not be affected by that change.

Though moving logic up the directory tree will lose some of the
hierarchy and might make it more difficult for people to determine where
a parent call was made.  Also changing the hierarchy like this seems
like trying to manage a scaling problem by coding details into random
makefiles.  By extension might this imply ~/Makefile.in in the sandbox
root eventually become a container makefile as logic migrates up the
tree closer to the sandbox root ?


 
> ::: build/autoconf/make-makefile
> @@ +15,5 @@
> >  # The Original Code is mozilla.org code.
> >  #
> >  # The Initial Developer of the Original Code is
> >  # Netscape Communications Corporation.
> > +# Portions created by the Initial Developer are Copyright (C) 1999-2011
> 
> There's no need to change the date here.  These don't actually mean anything
> (see bug 536336 for some fun legal reading).

Oh I'll peruse the legal-ease shortly


> ::: build/autoconf/make-makefile.excl
> @@ +1,5 @@
> > +###########################################################################
> > +## Intent: Exclusion list for container make builds
> > +###########################################################################
> > +
> > +# EOF
> 
> Why is this here?

js/src and nsprpub/ are causing problems for the intial deployment so an exclusion list has been setup.  Makefile.in within some subdirs are a special case setup by copying files and assigning DEPTH= as a subdirectory of the real sandbox top directory.  This has implications for some of the container-generated paths branded into the generated Makefile(s).  Exclude them in bulk for now and deal with the paths with rev 2.



> ::: build/autoconf/test/Makefile.in
> @@ +51,5 @@
> > +ifneq (,$(findstring check,$(MAKECMDGOALS)))
> > +          allsrc = $(wildcard $(srcdir)/*)
> > +       tests2run = $(notdir $(filter %.tpl,$(allsrc)))
> > +       tests2run += $(notdir $(filter %.tpm,$(allsrc)))
> > +  check_targets += $(addprefix $(TS)/,$(tests2run))
> 
> Consistent indentation please.

Copy & paste from earlier makefile edits, to be fixed shortly.  Most of the instances have been fixed so should show up less frequently going forward.


> 
> @@ +68,5 @@
> > +
> > +parent = $(patsubst %/,%,$(dir $(srcdir)))
> > +$(TS)/make-makefile.tpl: \
> > +  $(srcdir)/make-makefile.tpl\
> > +  $(parent)/makemakefile.pm\
> 
> Assuming you're trying to stay under 80 characters on this line, we
> generally just live with overlong lines in our Makefiles.
> 
> @@ +76,5 @@
> > +
> > +$(TS)/makemakefile.tpm: \
> > +  $(srcdir)/makemakefile.tpm \
> > +  $(parent)/makemakefile.pm \
> > +  $(NULL)
> 
> Same.

I find dep lists formatted one per line like this are infinitely easier to read than a stream of tokens that spans multiple lines.  Deps are legible not parsed out by $(SPACE) and typos are visible { like stray commas accidentally suffixed in a list of macro names creating non-existent file paths :) tough to spot in a long list of tokens }.


> 
> @@ +78,5 @@
> > +  $(srcdir)/makemakefile.tpm \
> > +  $(parent)/makemakefile.pm \
> > +  $(NULL)
> > +	$(PERL) $(srcdir)/runtest $<
> > +	@touch $@
> 
> Why do you have the wildcard target and then specific targets for .tpl/.tmp?
> Can't we just make the wildcard target depend on $(parent)/makemakefile.pm
> and be done?


> Why do you have the wildcard target and then specific targets for .tpl/.tmp? 
> Can't we just make the wildcard target depend on $(parent)/makemakefile.pm and
> be done?

It is not obvious with the current setup with only one test in the suite, but making the wildcard dependent on makemakefile.pm would allow running tests un-necessarily.  The rules are setup to only launch a test when either the test suite and/or module being guarded are modified.

If tests foo.tpm, bar.tpl, tans.pl were added to the directory.  A wildcard dep would trigger running all of the tests when makemakefile.pm is modified -- not just make-makefile.tpl to verify what has been changed.


That said in this instance the target rule could be made more generic.  If make-makefile and makemakefile.pm were renamed for consistency & '.pl' suffix the target rule would become:

    $(TS)/%.tpm: %.tpm ../%.pm
        runtest $<

also suitable for reuse as a common makefile library test target.



> ::: build/autoconf/test/make-makefile.excl
> @@ +4,5 @@
> > +
> > +/dev/null
> > +/foo/bar
> > +/a/b/c
> > +/a/b/d
> 
> And why is this here?

Unit test data.  The file exists for use by makemakefile.tpl::check_getExclusionList().  The test function will read this data and verify parsing/return values.



> 
> ::: js/src/build/autoconf/make-makefile.excl
> @@ +1,5 @@
> > +###########################################################################
> > +## Intent: Exclusion list for container make builds
> > +###########################################################################
> > +
> > +# EOF
> 
> Same question.

Hmmmm, I'll have to check this one.  I don't remember if check-sync-dirs required duplicating another file or a stray "rsync build/autoconf/make*
js/src/build/autoconf" copied it in.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-10 10:35:14 PDT
(In reply to Joey Armstrong [:joey] from comment #11)
> > ::: build/autoconf/Makefile.in
> > @@ +45,5 @@
> > > +
> > > +include $(DEPTH)/config/autoconf.mk
> > > +
> > > +ifdef ENABLE_TESTS
> > > +  DIRS += test
> > 
> > Rather than have this entire file, why not do
> > 
> > ifdef ENABLE_TESTS
> >   DIRS+=autoconf/tests
> > endif
> > 
> > in the parent directory?
> 
> The call could be moved not a big problem, more tests will be added
> beneath build/ at some point which should not be affected by that change.
> 
> Though moving logic up the directory tree will lose some of the
> hierarchy and might make it more difficult for people to determine where
> a parent call was made.  Also changing the hierarchy like this seems
> like trying to manage a scaling problem by coding details into random
> makefiles.  By extension might this imply ~/Makefile.in in the sandbox
> root eventually become a container makefile as logic migrates up the
> tree closer to the sandbox root ?

I'm still not entirely sure what a container makefile is, but yes, this is totally managing a scaling problem by coding details into random makefiles. :-)

> > ::: build/autoconf/make-makefile
> > @@ +15,5 @@
> > >  # The Original Code is mozilla.org code.
> > >  #
> > >  # The Initial Developer of the Original Code is
> > >  # Netscape Communications Corporation.
> > > +# Portions created by the Initial Developer are Copyright (C) 1999-2011
> > 
> > There's no need to change the date here.  These don't actually mean anything
> > (see bug 536336 for some fun legal reading).
> 
> Oh I'll peruse the legal-ease shortly

There's no need to actually read that (I just linked it case you were curious).

> > ::: build/autoconf/make-makefile.excl
> > @@ +1,5 @@
> > > +###########################################################################
> > > +## Intent: Exclusion list for container make builds
> > > +###########################################################################
> > > +
> > > +# EOF
> > 
> > Why is this here?
> 
> js/src and nsprpub/ are causing problems for the intial deployment so an
> exclusion list has been setup.  Makefile.in within some subdirs are a
> special case setup by copying files and assigning DEPTH= as a subdirectory
> of the real sandbox top directory.  This has implications for some of the
> container-generated paths branded into the generated Makefile(s).  Exclude
> them in bulk for now and deal with the paths with rev 2.

Can we add these files in a separate bug then?  They're not immediately relevant to this bug, and I have a feeling you're going to need more than just these two.

> > ::: build/autoconf/test/Makefile.in
> > @@ +51,5 @@
> > > +ifneq (,$(findstring check,$(MAKECMDGOALS)))
> > > +          allsrc = $(wildcard $(srcdir)/*)
> > > +       tests2run = $(notdir $(filter %.tpl,$(allsrc)))
> > > +       tests2run += $(notdir $(filter %.tpm,$(allsrc)))
> > > +  check_targets += $(addprefix $(TS)/,$(tests2run))
> > 
> > Consistent indentation please.
> 
> Copy & paste from earlier makefile edits, to be fixed shortly.  Most of the
> instances have been fixed so should show up less frequently going forward.
> 
> 
> > 
> > @@ +68,5 @@
> > > +
> > > +parent = $(patsubst %/,%,$(dir $(srcdir)))
> > > +$(TS)/make-makefile.tpl: \
> > > +  $(srcdir)/make-makefile.tpl\
> > > +  $(parent)/makemakefile.pm\
> > 
> > Assuming you're trying to stay under 80 characters on this line, we
> > generally just live with overlong lines in our Makefiles.
> > 
> > @@ +76,5 @@
> > > +
> > > +$(TS)/makemakefile.tpm: \
> > > +  $(srcdir)/makemakefile.tpm \
> > > +  $(parent)/makemakefile.pm \
> > > +  $(NULL)
> > 
> > Same.
> 
> I find dep lists formatted one per line like this are infinitely easier to
> read than a stream of tokens that spans multiple lines.  Deps are legible
> not parsed out by $(SPACE) and typos are visible { like stray commas
> accidentally suffixed in a list of macro names creating non-existent file
> paths :) tough to spot in a long list of tokens }.

OK.  I don't have strong feelings either way.

> > 
> > @@ +78,5 @@
> > > +  $(srcdir)/makemakefile.tpm \
> > > +  $(parent)/makemakefile.pm \
> > > +  $(NULL)
> > > +	$(PERL) $(srcdir)/runtest $<
> > > +	@touch $@
> > 
> > Why do you have the wildcard target and then specific targets for .tpl/.tmp?
> > Can't we just make the wildcard target depend on $(parent)/makemakefile.pm
> > and be done?
> 
> 
> > Why do you have the wildcard target and then specific targets for .tpl/.tmp? 
> > Can't we just make the wildcard target depend on $(parent)/makemakefile.pm and
> > be done?
> 
> It is not obvious with the current setup with only one test in the suite,
> but making the wildcard dependent on makemakefile.pm would allow running
> tests un-necessarily.  The rules are setup to only launch a test when either
> the test suite and/or module being guarded are modified.
> 
> If tests foo.tpm, bar.tpl, tans.pl were added to the directory.  A wildcard
> dep would trigger running all of the tests when makemakefile.pm is modified
> -- not just make-makefile.tpl to verify what has been changed.

Ah, ok.

> That said in this instance the target rule could be made more generic.  If
> make-makefile and makemakefile.pm were renamed for consistency & '.pl'
> suffix the target rule would become:
> 
>     $(TS)/%.tpm: %.tpm ../%.pm
>         runtest $<
> 
> also suitable for reuse as a common makefile library test target.

This might be worth moving into rules.mk at some point in the future.

> > ::: build/autoconf/test/make-makefile.excl
> > @@ +4,5 @@
> > > +
> > > +/dev/null
> > > +/foo/bar
> > > +/a/b/c
> > > +/a/b/d
> > 
> > And why is this here?
> 
> Unit test data.  The file exists for use by
> makemakefile.tpl::check_getExclusionList().  The test function will read
> this data and verify parsing/return values.

Ok.

> > 
> > ::: js/src/build/autoconf/make-makefile.excl
> > @@ +1,5 @@
> > > +###########################################################################
> > > +## Intent: Exclusion list for container make builds
> > > +###########################################################################
> > > +
> > > +# EOF
> > 
> > Same question.
> 
> Hmmmm, I'll have to check this one.  I don't remember if check-sync-dirs
> required duplicating another file or a stray "rsync build/autoconf/make*
> js/src/build/autoconf" copied it in.
Comment 13 Joey Armstrong [:joey] 2011-10-17 10:51:24 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> (In reply to Joey Armstrong [:joey] from comment #11)
> > > ::: build/autoconf/Makefile.in
> > > @@ +45,5 @@
> > > Why is this here?
> > 
> > js/src and nsprpub/ are causing problems for the intial deployment so an
> > exclusion list has been setup.  Makefile.in within some subdirs are a
> > special case setup by copying files and assigning DEPTH= as a subdirectory
> > of the real sandbox top directory.  This has implications for some of the
> > container-generated paths branded into the generated Makefile(s).  Exclude
> > them in bulk for now and deal with the paths with rev 2.
> 
> Can we add these files in a separate bug then?  They're not immediately
> relevant to this bug, and I have a feeling you're going to need more than
> just these two.

New bugs can/should be opened for tracking the directories but exclusions will need to be listed in the exclusion file as this is an all or nothing op for submitting changes.

If problem makefiles are not excluded they will be regenerated by the 'container make call'.  DEPTH=/path problems in the generated makefiles will cause the downstream make export/libs/tools call to fail.

ps>  I did compare makefiles from a clean sandbox with files generated by this patch and nsprpub/ and js/src were the only directories that have reported deltas.  All of the others generated verbatim makefiles.
Comment 14 Joey Armstrong [:joey] 2011-10-24 07:21:40 PDT
Created attachment 569059 [details] [diff] [review]
edits for using make-makefile from sandbox root & unit tests added.

Latest try job: http://tbpl.mozilla.org/?tree=Try&rev=181376ed37fd

Moved build/autoconf/Makefile.in logic into build/Makefile.in per Kyle.

Full rebuild in two sandboxes.  Compared generated makefiles ignoring topsrc= and VPATH= paths.  Only delta detected was the new unit test makefile build/autoconf/test/Makefile.in.
Comment 15 Joey Armstrong [:joey] 2011-10-24 07:39:27 PDT
fyi> The only edit from the last approved patch was removing build/autoconf/Makefile.in.
Comment 16 Dão Gottwald [:dao] 2011-10-24 08:19:57 PDT
Comment on attachment 569059 [details] [diff] [review]
edits for using make-makefile from sandbox root & unit tests added.

>--- a/build/autoconf/make-makefile
>+++ b/build/autoconf/make-makefile

>+#   Steve Lamm (slamm@netscape.com).
>+#   Joey Armstrong <joey@mozilla.com>

The first line should use angle brackets as well, and the stray dot should be removed.
Comment 17 Joey Armstrong [:joey] 2011-10-24 08:40:24 PDT
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 569059 [details] [diff] [review] [diff] [details] [review]
> edits for using make-makefile from sandbox root & unit tests added.
> 
> >--- a/build/autoconf/make-makefile
> >+++ b/build/autoconf/make-makefile
> 
> >+#   Steve Lamm (slamm@netscape.com).
> >+#   Joey Armstrong <joey@mozilla.com>
> 
> The first line should use angle brackets as well, and the stray dot should
> be removed.

Format was taken from an existing comment line in the file.  Just moved to the "contributor" section.
Comment 18 Dão Gottwald [:dao] 2011-10-30 07:52:35 PDT
(In reply to Joey Armstrong [:joey] from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > Comment on attachment 569059 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > edits for using make-makefile from sandbox root & unit tests added.
> > 
> > >--- a/build/autoconf/make-makefile
> > >+++ b/build/autoconf/make-makefile
> > 
> > >+#   Steve Lamm (slamm@netscape.com).
> > >+#   Joey Armstrong <joey@mozilla.com>
> > 
> > The first line should use angle brackets as well, and the stray dot should
> > be removed.
> 
> Format was taken from an existing comment line in the file.  Just moved to
> the "contributor" section.

There was no "format" where you copied this from, it was a sentence.
Comment 19 Joey Armstrong [:joey] 2011-10-31 11:23:22 PDT
>> Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-10 10:36:11 PDT
>> Attachment #564961 [details] [diff] - Flags: review?(khuey@kylehuey.com) ➔ review+

Kyle, can the review from the 10th be carried forward on this patch ?

The only real delta was removing build/autoconf/Makefile.in and adding DIRS += in build/Makefile.in.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-01 10:43:34 PDT
(In reply to Joey Armstrong [:joey] from comment #19)
> >> Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-10 10:36:11 PDT
> >> Attachment #564961 [details] [diff] - Flags: review?(khuey@kylehuey.com) ➔ review+
> 
> Kyle, can the review from the 10th be carried forward on this patch ?

Yes.
Comment 21 Chris Cooper [:coop] [away until Aug 29] 2011-11-08 07:18:33 PST
Comment on attachment 569059 [details] [diff] [review]
edits for using make-makefile from sandbox root & unit tests added.

Review from khuey carried forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fced060132c3
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fced060132c3
Comment 22 Ed Morley [:emorley] 2011-11-08 07:26:59 PST
(In reply to Chris Cooper [:coop] from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fced060132c3

Is this cset a followup to something? The commit message implies so, but I can't see another cset listed in this bug. Either an initial landing cset is missing higher up in this bug, or else the commit message isn't correct perhaps?
Comment 23 Ed Morley [:emorley] 2011-11-08 07:29:36 PST
(In reply to Ed Morley [:edmorley] from comment #22)
> or else the commit message isn't correct perhaps?

(or else I've completely misunderstood comment 0 hehe :-))
Comment 24 Joey Armstrong [:joey] 2011-11-08 07:34:47 PST
(In reply to Ed Morley [:edmorley] from comment #23)
> (In reply to Ed Morley [:edmorley] from comment #22)
> > or else the commit message isn't correct perhaps?
> 
> (or else I've completely misunderstood comment 0 hehe :-)

The change set is a minor delta for the approved patch obsoleted between comments 14 & 15.
Comment 25 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-08 08:51:54 PST
Unfortunately we had to back this out because FindBin fails when it's used in a script invoked from a Windows path (which happens in pymake builds :-/).

https://hg.mozilla.org/integration/mozilla-inbound/rev/dacf503b344b
Comment 26 Chris Cooper [:coop] [away until Aug 29] 2011-11-08 08:57:09 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #25)
> Unfortunately we had to back this out because FindBin fails when it's used
> in a script invoked from a Windows path (which happens in pymake builds :-/).
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dacf503b344b

So how should Joey go about fixing this? Does Joey have the pymake build instructions he needs to get this working?

I find it more than a little annoying that Joey's having work yanked based on code that isn't officially part of the tree yet. I understand that devs are going to want to use the fastest build process possible, especially on Windows, but this seems like unfair sandbagging to Joey.
Comment 27 Chris Cooper [:coop] [away until Aug 29] 2011-11-08 09:01:31 PST
Comment on attachment 569059 [details] [diff] [review]
edits for using make-makefile from sandbox root & unit tests added.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dacf503b344b
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-08 09:12:25 PST
(In reply to Chris Cooper [:coop] from comment #26)
> So how should Joey go about fixing this? Does Joey have the pymake build
> instructions he needs to get this working?

I'm not sure.  The problem seems to be in the modules shipped with Perl.  Can we avoid using this module easily?

> I find it more than a little annoying that Joey's having work yanked based
> on code that isn't officially part of the tree yet. I understand that devs
> are going to want to use the fastest build process possible, especially on
> Windows, but this seems like unfair sandbagging to Joey.

I'm not sure what "officially part of the tree" means here.  Pymake might not be what the build machines are using, but it is what the developers are using on Windows.  The situation here is hardly ideal, but given the choice between landing this patch and breaking all of our Windows developers and backing it out and fixing it I think the choice is pretty clear.
Comment 29 Ted Mielczarek [:ted.mielczarek] 2011-11-08 10:09:38 PST
And note that we are actively working on making pymake the default configuration on tinderbox, so breaking it makes that work harder as well.
Comment 30 Chris Cooper [:coop] [away until Aug 29] 2011-11-08 10:42:24 PST
(In reply to Ted Mielczarek [:ted, :luser] from comment #29)
> And note that we are actively working on making pymake the default
> configuration on tinderbox, so breaking it makes that work harder as well.

So that raises the obvious question: *when* are we planning to make pymake the default? 

Is a possible solution to get Joey access to the build-system project branch? How often does work from that branch get uplifted to m-c?
Comment 31 Gregory Szorc [:gps] 2011-11-08 12:45:45 PST
(In reply to Chris Cooper [:coop] from comment #30)
> So that raises the obvious question: *when* are we planning to make pymake
> the default? 

The transition can occur as soon as the build system fully works on PyMake and "we" want to flip that switch, AFAICT. Bug 593585 tracks flipping that switch. It seems pretty close...
Comment 32 Joey Armstrong [:joey] 2011-11-10 10:30:55 PST
Created attachment 573565 [details] [diff] [review]
original make-makefile enhancements + fix for pymake

Failure reported for building when msys-perl is invoked from windows command shell { when launched by pymake }.

If perl binary is msys and searchpath contains a drive colon massage the value of $0 from C: into /c/foo so core modules will be able to perform string manipulations to determine script startup directory and other values.
Comment 33 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-10 10:33:57 PST
Can you post a diff showing just the changes from the last version?
Comment 34 Joey Armstrong [:joey] 2011-11-10 10:39:24 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #33)
> Can you post a diff showing just the changes from the last version?



This should be the bulk of edits but I'll check on generating a full diff from my patch queue.

build/autoconf/make-makefile
js/src/build/autoconf/make-makefile
===================================

< ##############################################################
< # pymake: special case path handling for windows cmd shell.
< #   if invoked by cmd.exe and msys-perl is in play
< #     $0 may contain a drive letter
< #     modules use-or-expect msys/unix paths
< #     adjust $0 => C:/foo => /c/foo so string tests and
< #     manipulation can by applied properly.
< ##############################################################
< sub BEGIN
< {
<     if ($^O eq 'msys' && $ENV{PATH} =~ m!\w:/!)
<     {
<       $0 =~ s!^(\w):!/$1!;
<     }
<     eval 'use FindBin';
<     die $@ if ($@);
< }
<
Comment 35 Joey Armstrong [:joey] 2011-11-10 10:42:23 PST
A try job passed earlier.  This is a resubmit of the patch with debug lines removed.
https://tbpl.mozilla.org/?tree=Try&rev=217b96bb3dd9
Comment 36 Joey Armstrong [:joey] 2011-11-10 11:49:22 PST
Created attachment 573585 [details]
patch diff

diff of old patch against patch + pymake fix
Comment 37 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-10 11:52:37 PST
Comment on attachment 573565 [details] [diff] [review]
original make-makefile enhancements + fix for pymake

Thanks.
Comment 38 Chris Cooper [:coop] [away until Aug 29] 2011-11-10 20:28:27 PST
Comment on attachment 573565 [details] [diff] [review]
original make-makefile enhancements + fix for pymake

No issues here.
Comment 39 Dão Gottwald [:dao] 2011-11-19 04:04:55 PST
Created attachment 575649 [details] [diff] [review]
original make-makefile enhancements + fix for pymake

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Joey Armstrong [:joey] from comment #17)
> > (In reply to Dão Gottwald [:dao] from comment #16)
> > > Comment on attachment 569059 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] [diff] [details] [review]
> > > edits for using make-makefile from sandbox root & unit tests added.
> > > 
> > > >--- a/build/autoconf/make-makefile
> > > >+++ b/build/autoconf/make-makefile
> > > 
> > > >+#   Steve Lamm (slamm@netscape.com).
> > > >+#   Joey Armstrong <joey@mozilla.com>
> > > 
> > > The first line should use angle brackets as well, and the stray dot should
> > > be removed.
> > 
> > Format was taken from an existing comment line in the file.  Just moved to
> > the "contributor" section.
> 
> There was no "format" where you copied this from, it was a sentence.

Fixed.

Fixed the commit message, too.
Comment 40 Ed Morley [:emorley] 2011-11-19 18:10:11 PST
Didn't apply cleanly to inbound, but fixed locally just so we can get this landed.

Now in my queue with a few other bits that are being sent to try first and then onto inbound :-)

https://tbpl.mozilla.org/?tree=Try&rev=eb96679e6888
Comment 41 Ed Morley [:emorley] 2011-11-19 18:27:35 PST
Failed check-sync-dirs due to the email comment changes. Have sync'd build/ and js/build/ as well as corrected a two other instances of emails being in brackets.

New try run:
https://tbpl.mozilla.org/?tree=Try&rev=329bc2d383a3
Comment 43 Ed Morley [:emorley] 2011-11-20 14:19:41 PST
https://hg.mozilla.org/mozilla-central/rev/bf262993def8
Comment 44 Ed Morley [:emorley] 2011-12-03 13:20:09 PST
build/autoconf/test/Makefile was created in the tree by this bug, but was not listed in allmakefiles.sh, so I've added it in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

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