Closed Bug 680369 Opened 13 years ago Closed 12 years ago

Make Services Makefiles More Intelligent

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 759487

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
(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.
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
Depends on: 680534
Here is another incremental patch to make things better. It isn't complete, but is getting better.
Attachment #554330 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
No longer depends on: 680536, 701393
Resolution: --- → DUPLICATE
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.

Attachment

General

Created:
Updated:
Size: