Make Services Makefiles More Intelligent

RESOLVED DUPLICATE of bug 759487

Status

()

RESOLVED DUPLICATE of bug 759487
8 years ago
4 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
There are some annoyances in the m-c/services Makefiles, mainly that a `make` on a fully pre-built and unchanged tree results in recipes being executed because the targets are defined non-ideally.

This bugs seeks to improve the Makefiles so that a `make` on an already-built tree just prints a bunch of "Nothing to be done..." messages and exits.

Submitted patches should have the output objdir tree compared with that of the current version to ensure there is no discrepancy. Patches should also be tested against both make and PyMake.
(Assignee)

Comment 1

8 years ago
Created attachment 554330 [details] [diff] [review]
Improvements to services/sync Makefile

This changes the services/sync Makefile to use proper targets for directories and implicit rules on .js files. Since the targets are defined properly, no action is taken if everything has been done already. Contrast this to the previous Makefile, where the "libs" targets was defined with "::", which meant it executed every time.

I'm pretty sure I managed to bust some functionality with this patch. But, I thought I'd upload something to track the progress. I have tested on GNU make, but not PyMake (PyMake supports implicit rules, so it should work fine).

Here is some sample output:

gps@ubuntu:~/src/services-central/obj-ff-debug/services/sync$ make
make: Nothing to be done for `sync'.

gps@ubuntu:~/src/services-central/obj-ff-debug/services/sync$ touch ../../../services/sync/modules/engines/history.js
gps@ubuntu:~/src/services-central/obj-ff-debug/services/sync$ make
/usr/bin/python2.7 /home/gps/src/services-central/config/Preprocessor.py -Dweave_version=1.11.0 -Dweave_channel=rel -Dweave_id={340c2bbc-ce74-4362-90b5-7c26312808ef} /home/gps/src/services-central/services/sync/modules/engines/history.js > ../../dist/bin/modules/services-sync/engines/history.js
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment on attachment 554330 [details] [diff] [review]
Improvements to services/sync Makefile

Awesome. Some feedback:

> # Definitions used by constants.js.
> WEAVE_VERSION=$(shell cat $(topsrcdir)/services/sync/version.txt)
> WEAVE_CHANNEL=rel
> WEAVE_ID={340c2bbc-ce74-4362-90b5-7c26312808ef}

WEAVE_CHANNEL and WEAVE_ID can go away. They're relics from the old extension co-existence.

>+module_dirs := engines ext

I would actually like to get rid of 'ext', too. We could easily combine the modules there into one top-level 'helpers.js' file.

>+$(FINAL_TARGET)/modules/services-sync/%.js: %.js
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(SYNC_PP_DEFINES) $< > $@

Pretty sure we only need to run the preprocessor for is constants.js (which should really be called constants.js.in for that reason). For all other files we could continue to use nsinstall which would give us symlinks on 'nix, I think. Which means not having to rebuild when you change a file.

The preprocessor-based files should also depend on version.txt, though, because that's where we get WEAVE_VERSION from.
(Assignee)

Comment 3

8 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> I would actually like to get rid of 'ext', too. We could easily combine the
> modules there into one top-level 'helpers.js' file.

Why should we break blame and creep scope? If you really want to do this, perhaps opening a new bug is the right course. If it gets committed before this one, I'll address it.
 
> The preprocessor-based files should also depend on version.txt, though,
> because that's where we get WEAVE_VERSION from.

Good catch.
(In reply to Gregory Szorc [:gps] from comment #3)
> (In reply to Philipp von Weitershausen [:philikon] from comment #2)
> > I would actually like to get rid of 'ext', too. We could easily combine the
> > modules there into one top-level 'helpers.js' file.
> 
> Why should we break blame and creep scope? If you really want to do this,
> perhaps opening a new bug is the right course. If it gets committed before
> this one, I'll address it.

Yeah, sorry, didn't mean to creep scope for this bug. I should've said that that's for another bug. One of the reasons why I haven't filed it yet is that I still can't make my mind up about whether I want to get rid of some of those helpers altogether. HULK SMASH LAYERS.
(Assignee)

Comment 5

8 years ago
I'm defining the scope of this bug to be:

"A `make -C services` inside a fully-built object directory should result in no rules with recipes being evaluated." In other words, "a `make -C services` should only result in the following output patterns from make: 'Entering directory,' 'Nothing to be done for,' and 'Leaving directory.'"

Where there are blockers in included makefiles (such as rules.mk), I will file bugs against them (possibly creating patches in the process) and mark them as blockers to this bug.
Depends on: 680536
(Assignee)

Updated

8 years ago
Depends on: 680534
(Assignee)

Comment 6

8 years ago
Created attachment 554960 [details] [diff] [review]
Improvements to Makefiles, v1

Here is another incremental patch to make things better. It isn't complete, but is getting better.
Attachment #554330 - Attachment is obsolete: true
(Assignee)

Comment 7

7 years ago
Since I started work on this, I learned that declarative Makefiles are preferred. So, once bug 701393 is ironed out, I'll take a stab at making these fully declarative. This means we'll have to update the Makefile when adding new .js files, but such is life.
Depends on: 701393
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
No longer depends on: 680536, 701393
Resolution: --- → DUPLICATE
Duplicate of bug: 759487
Component: Firefox Sync: Build → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.