Closed Bug 912293 Opened 11 years ago Closed 11 years ago

Add a generic header and footer to generated Makefiles

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
Blocks: 912292
Attachment #799180 - Flags: review?(gps)
I want to remove the blank lines this removes before putting this up for review.
This also takes into account the makefiles we have in CONFIGURE_SUBST_FILES. (without this, make package fails, for instance)
Attachment #799198 - Flags: review?(gps)
Attachment #799180 - Attachment is obsolete: true
Attachment #799180 - Flags: review?(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"?
Attachment #799198 - Flags: review?(gps) → review+
Attachment #799223 - Flags: review?(gps)
Attachment #799182 - Attachment is obsolete: true
Attachment #799223 - Flags: review?(gps)
Attachment #799231 - Flags: review?(gps)
Attachment #799223 - Attachment is obsolete: true
Removed a spurious DEPTH in js/xpconnect/loader/Makefile.in
Attachment #799233 - Flags: review?(gps)
Attachment #799231 - Attachment is obsolete: true
Attachment #799231 - Flags: review?(gps)
Comment on attachment 799233 [details] [diff] [review]
Remove now redundant boilerplate from Makefile.in

sigh. it's now broken for some reason :(
Attachment #799233 - Flags: review?(gps)
I /think/ this is the one. Giving it a try spin.
Attachment #799233 - Attachment is obsolete: true
Depends on: 912368
A couple changes to make config.status --file do kind of the right thing.
Attachment #799326 - Flags: review?(gps)
Attachment #799198 - Attachment is obsolete: true
Hopefully this one is good, although it's not against current m-i and will need some merge conflict resolution.
Attachment #799246 - Attachment is obsolete: true
(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.
(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 ;)
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 on attachment 799327 [details] [diff] [review]
Remove now redundant boilerplate from Makefile.in

This is the one.
Attachment #799327 - Flags: review?(gps)
Thanks for the consideration, both :)
Attachment #799326 - Flags: review?(gps) → review+
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.
Attachment #799327 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/92ee006ec0a5
https://hg.mozilla.org/mozilla-central/rev/45097bc3a578
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 920223
Blocks: 773349
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: