Remove empty Makefile.in

RESOLVED FIXED in mozilla22

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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!
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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?
(Assignee)

Comment 3

5 years ago
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.
Assignee: nobody → gps
Attachment #717648 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #717663 - Flags: review?(mh+mozilla)
(Assignee)

Comment 4

5 years ago
Created attachment 717664 [details] [diff] [review]
Part 2: Allow missing Makefile.in, v1

Alters the RecursiveMakeBackend so missing Makefile.in are handled properly.
Attachment #717664 - Flags: review?(mh+mozilla)
(Assignee)

Comment 5

5 years ago
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!
Attachment #717665 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

5 years ago
Try at https://tbpl.mozilla.org/?tree=Try&rev=21c65ac3a59c
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.
Attachment #717663 - Flags: review?(mh+mozilla) → review-
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.
Attachment #717664 - Flags: review?(mh+mozilla) → review+
Attachment #717665 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #717663 - Attachment is obsolete: true
Attachment #719823 - Flags: review?(mh+mozilla)
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
Attachment #719823 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
Inbound has been closed all day. Why not use the build-system branch for this?

https://hg.mozilla.org/projects/build-system/rev/faba12426a13
https://hg.mozilla.org/projects/build-system/rev/05914d4f27e3
https://hg.mozilla.org/projects/build-system/rev/231b7f8046d9
Whiteboard: [fixed-in-build-system]
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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
Whiteboard: [fixed-in-build-system] → [leave open]
https://hg.mozilla.org/mozilla-central/rev/faba12426a13
(Assignee)

Comment 17

5 years ago
Over to joey to figure out bustage.
Assignee: gps → joey
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
(Assignee)

Updated

5 years ago
Depends on: 851975
(Assignee)

Comment 19

5 years ago
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.
Assignee: joey → gps
(Assignee)

Comment 20

5 years ago
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
Whiteboard: [leave open]
Target Milestone: --- → mozilla22

Comment 21

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/3c87e1fe617d
https://hg.mozilla.org/mozilla-central/rev/336b6586074e
https://hg.mozilla.org/mozilla-central/rev/0a0951f42b0b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Blocks: 873629
You need to log in before you can comment on or make changes to this bug.