912293 - Add a generic header and footer to generated Makefiles

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

For bug 912292, I needed to reliably add something to all makefiles, either at the beginning or the end. Since I was doing this, I figured I might as well just add the usual boilerplate, and remove it from Makefile.in.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Add a generic header and footer to generated Makefiles

Comment 2

[Mike Hommey [:glandium]]

Attachment: Remove now redundant boilerplate from Makefile.in

I want to remove the blank lines this removes before putting this up for review.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Add a generic header and footer to generated Makefiles

This also takes into account the makefiles we have in CONFIGURE_SUBST_FILES. (without this, make package fails, for instance)

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 799198 [details] [diff] [review]
Add a generic header and footer to generated Makefiles

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

\o/ would review again.

::: js/src/configure.in
@@ -4327,5 @@
>  
>  dnl Spit out some output
>  dnl ========================================================
>  
> -AC_OUTPUT([js-confdefs.h Makefile config/autoconf.mk config/emptyvars.mk])

I'm surprised this lasted as long as it did!

::: python/mozbuild/mozbuild/backend/configenvironment.py
@@ +227,5 @@
> +        pp.handleLine('include $(DEPTH)/config/autoconf.mk\n')
> +        if not stub:
> +            pp.do_include(self.get_input(path))
> +        pp.handleLine('\n') # Empty line to avoid failures when last line
> +        # in Makefile.in ends with a backslash.

Nit: Move comment to own line(s)

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +175,5 @@
>  
>          if isinstance(obj, DirectoryTraversal):
>              self._process_directory_traversal(obj, backend_file)
>          elif isinstance(obj, ConfigFileSubstitution):
> +            if obj.output_path.endswith('/Makefile'):

Is relying on / safe here? Should we throw in a .replace('/', os.sep) just to be sure? Maybe we should do that in emitter.py if we're not already?

@@ +179,5 @@
> +            if obj.output_path.endswith('/Makefile'):
> +                f = backend_file.environment.create_makefile(obj.output_path)
> +            else:
> +                f = backend_file.environment.create_config_file(obj.output_path)
> +            self._update_from_avoid_write(f)

This originally confused me because I thought f was a file and not a FileAvoidWrite result. Change variable to "result" or "r"?

Comment 5

[Mike Hommey [:glandium]]

Attachment: Remove now redundant boilerplate from Makefile.in

Comment 6

[Mike Hommey [:glandium]]

Attachment: Remove now redundant boilerplate from Makefile.in

Comment 7

[Mike Hommey [:glandium]]

Attachment: Remove now redundant boilerplate from Makefile.in

Removed a spurious DEPTH in js/xpconnect/loader/Makefile.in

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 799233 [details] [diff] [review]
Remove now redundant boilerplate from Makefile.in

sigh. it's now broken for some reason :(

Comment 9

[Mike Hommey [:glandium]]

Attachment: Remove now redundant boilerplate from Makefile.in

I /think/ this is the one. Giving it a try spin.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Add a generic header and footer to generated Makefiles

A couple changes to make config.status --file do kind of the right thing.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Remove now redundant boilerplate from Makefile.in

Hopefully this one is good, although it's not against current m-i and will need some merge conflict resolution.

Comment 12

[Ed Morley [:emorley]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Created attachment 799327 [details] [diff] [review]
> Remove now redundant boilerplate from Makefile.in
> 
> Hopefully this one is good, although it's not against current m-i and will
> need some merge conflict resolution.

Makefiles under dom/imptests/*/ are automatically generated - http://mxr.mozilla.org/mozilla-central/source/dom/imptests/writeBuildFiles.py will need to be updated.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #12)
> Makefiles under dom/imptests/*/ are automatically generated -
> http://mxr.mozilla.org/mozilla-central/source/dom/imptests/writeBuildFiles.
> py will need to be updated.

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=912293&attachment=799327 ;)

Comment 14

[Ed Morley [:emorley]]

Ah dammit, case sensitive sort order in https://bugzilla.mozilla.org/attachment.cgi?id=799327&action=diff after using "collapse all"; saw dom/imptests/Makefile.in and didn't look below the directories under that. Sorry! :-)

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 799327 [details] [diff] [review]
Remove now redundant boilerplate from Makefile.in

This is the one.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks for the consideration, both :)

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 799327 [details] [diff] [review]
Remove now redundant boilerplate from Makefile.in

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

I only glanced at large parts of this patch because OMG IT MAKES MY EYES BLEED.

But, I did have a look at a few paths that interest me, notably dom/imptests and config/makefiles.

Comment 18

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92ee006ec0a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/45097bc3a578

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92ee006ec0a5
https://hg.mozilla.org/mozilla-central/rev/45097bc3a578