Last Comment Bug 844635 - Remove empty Makefile.in
: Remove empty Makefile.in
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Gregory Szorc [:gps]
:
Mentors:
Depends on: 784841 851975
Blocks: 873629
  Show dependency treegraph
 
Reported: 2013-02-24 10:29 PST by Gregory Szorc [:gps]
Modified: 2013-05-17 12:54 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: mach command to identify empty Makefile.in, v1 (2.12 KB, patch)
2013-02-24 10:58 PST, Gregory Szorc [:gps]
no flags Details | Diff | Review
Part 1: mach command to identify empty Makefile.in, v2 (2.15 KB, patch)
2013-02-24 12:32 PST, Gregory Szorc [:gps]
mh+mozilla: review-
Details | Diff | Review
Part 2: Allow missing Makefile.in, v1 (8.14 KB, patch)
2013-02-24 12:33 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Review
Part 3: Remove empty Makefile.in, v1 (71.93 KB, patch)
2013-02-24 12:35 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Review
Part 1: mach command to identify empty Makefile.in, v3 (2.33 KB, patch)
2013-02-28 21:34 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Review

Description Gregory Szorc [:gps] 2013-02-24 10:29:14 PST
As part of the initial switchover to moz.build files in bug 784841, I purposely left "empty" Makefile.in around so it would be easier to notice orphaned files and directories as part of the transition.

Once the initial switchover is complete, we should remove these "empty" Makefile.in files and have the build backend generate a stub Makefile.

By "empty" Makefile.in, I'm referring to files that are essentially:

---
# 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/.

DEPTH     = @DEPTH@
topsrcdir = @top_srcdir@
srcdir    = @srcdir@
VPATH     = @srcdir@

include $(DEPTH)/config/autoconf.mk

include $(topsrcdir)/config/rules.mk

---

I may code up a quick script to use the pymake API to help identify empty Makefile.in. We may even hook it into the build to ensure empty Makefile.in don't linger in the tree!
Comment 1 Gregory Szorc [:gps] 2013-02-24 10:58:09 PST
Created attachment 717648 [details] [diff] [review]
Part 1: mach command to identify empty Makefile.in, v1

This is crude and seems to do a decent job. It currently identifies ~130 Makefile.in as empty.
Comment 2 Gregory Szorc [:gps] 2013-02-24 11:58:39 PST
Hmmm. This change may require nuking the "$(SUBMAKEFILES): % : $(srcdir)/%.in" rule or at least altering it.

Since make knows how to handle Makefile.in updates and can reinvoke itself, I believe the only thing SUBMAKEFILES provides is the ability to generate the Makefile in case it isn't there before we start traverse. I believe the only time this comes into play is incremental builds after updating the tree. In a moz.build world, the file linking to the new directory would incur a full tree scan and backend generation, installing the new Makefile. So, I /think/ I just justified the removal of the SUBMAKEFILES rule (at least for moz.build derived Makefiles).

Am I missing anything?
Comment 3 Gregory Szorc [:gps] 2013-02-24 12:32:32 PST
Created attachment 717663 [details] [diff] [review]
Part 1: mach command to identify empty Makefile.in, v2

Now with relativesrcdir detection. Can you think of anything else to add?

I anticipate this command expanding in the future to help identify "short" Makefile.in. We could use the output of that to determine which variables are "holding us back" and prioritize porting them to the new world order.
Comment 4 Gregory Szorc [:gps] 2013-02-24 12:33:23 PST
Created attachment 717664 [details] [diff] [review]
Part 2: Allow missing Makefile.in, v1

Alters the RecursiveMakeBackend so missing Makefile.in are handled properly.
Comment 5 Gregory Szorc [:gps] 2013-02-24 12:35:24 PST
Created attachment 717665 [details] [diff] [review]
Part 3: Remove empty Makefile.in, v1

Nuked a bunch of files. Also updated CLOBBER because this will require a new backend generation. (It appears support for detecting changes to the actual mozbuild .py files and triggering rebuild has been lost in recent versions of patches in bug 784841). We should fix in bug 784841 or file a follow-up. I vote for the latter because I want to land 784841!
Comment 6 Gregory Szorc [:gps] 2013-02-24 12:37:47 PST
Try at https://tbpl.mozilla.org/?tree=Try&rev=21c65ac3a59c
Comment 7 Mike Hommey [:glandium] 2013-02-25 04:02:55 PST
Comment on attachment 717663 [details] [diff] [review]
Part 1: mach command to identify empty Makefile.in, v2

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +288,5 @@
> +            'DEPTH',
> +            'topsrcdir',
> +            'srcdir',
> +            'relativesrcdir',
> +            'VPATH',

The values for these variables should be checked, too. Specifically, it would be bad to declare empty Makefiles where relativesrcdir is not the expected value (we have a bunch of those, unfortunately). There are also places where VPATH is different, too.
Comment 8 Mike Hommey [:glandium] 2013-02-25 04:04:28 PST
On a more general note, maybe this mach command should be a sub-command, like mach debug empty-makefiles or something like that, so that adding such sanity check commands won't clutter the user-visible documentation.
Comment 9 Gregory Szorc [:gps] 2013-02-25 09:46:22 PST
(In reply to Mike Hommey [:glandium] from comment #8)
> On a more general note, maybe this mach command should be a sub-command,
> like mach debug empty-makefiles or something like that, so that adding such
> sanity check commands won't clutter the user-visible documentation.

I can't remember if this bug is on file, but I anticipate adding a "category" decorator to mach commands so related commands are sorted in the UI. Along that vein, we may also expose a visibility level so low-level commands aren't visible by default. This is similar to how git and hg work.
Comment 10 Gregory Szorc [:gps] 2013-02-28 21:34:13 PST
Created attachment 719823 [details] [diff] [review]
Part 1: mach command to identify empty Makefile.in, v3

Now checking values of variables against a whitelist of known ignorable values, per review.

If all goes well, I'll probably land this on inbound Friday.
Comment 11 Mike Hommey [:glandium] 2013-03-01 00:01:52 PST
Comment on attachment 719823 [details] [diff] [review]
Part 1: mach command to identify empty Makefile.in, v3

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

r=me with that fixed.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +284,5 @@
> +        import pymake.parser
> +        import pymake.parserdata
> +
> +        IGNORE_VARIABLES = {
> +            'DEPTH': ('@DEPTH@'),

Seeing the s.value not in IGNORE_VARIABLES[exp.s] below, it seems you're assuming ('foo') creates a tuple, but it doesn't.

>>> type(('foo'))
<type 'str'>
>>> type(('foo',))
<type 'tuple'>

>>> 'f' in ('foo')
True
>>> 'f' in ('foo',)
False
Comment 12 Gregory Szorc [:gps] 2013-03-01 08:57:47 PST
(In reply to Mike Hommey [:glandium] from comment #11)
> Seeing the s.value not in IGNORE_VARIABLES[exp.s] below, it seems you're
> assuming ('foo') creates a tuple, but it doesn't.

If I had a dollar every time I got bit by this.
Comment 14 Gregory Szorc [:gps] 2013-03-01 17:50:03 PST
We've got a new mochitest (4) failure on build-system:

TEST-UNEXPECTED-FAIL | /tests/dom/workers/test/test_xhr_system.html | undefined - got 404, expected 200

Most interesting.
Comment 15 Gregory Szorc [:gps] 2013-03-02 17:38:26 PST
I backed out parts 2 and 3 for busting the tree. I can repro locally. So, it's just a matter of me tracking this down.

https://hg.mozilla.org/projects/build-system/rev/96109ede2a79
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-03-02 20:51:43 PST
https://hg.mozilla.org/mozilla-central/rev/faba12426a13
Comment 17 Gregory Szorc [:gps] 2013-03-14 10:51:44 PDT
Over to joey to figure out bustage.
Comment 18 Joey Armstrong [:joey] 2013-03-15 13:45:49 PDT
May be more than test failures in play, backend.mk files are not being generated.

% mach build
 0:00.36 844635/config/rules.mk:44: backend.mk: No such file or directory
 0:00.36 gmake[2]: *** No rule to make target `backend.mk'.  Stop.
 0:00.36 gmake[1]: *** [realbuild] Error 2
Comment 19 Gregory Szorc [:gps] 2013-03-17 17:10:35 PDT
The failure was caused by bug 851975. I'm pretty sure little to no further work is required on this bug. I'll land parts 2 and 3 once the dust settles from bug 851975.
Comment 20 Gregory Szorc [:gps] 2013-03-17 18:03:14 PDT
Well, now that bug 851975 took care of the issue with the mochitest:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c87e1fe617d
https://hg.mozilla.org/integration/mozilla-inbound/rev/336b6586074e
Comment 21 Mozilla RelEng Bot 2013-03-17 21:30:36 PDT
Try run for 356dfaada5ed is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=356dfaada5ed
Results (out of 313 total builds):
    success: 288
    warnings: 23
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-356dfaada5ed

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