Closed Bug 759487 Opened 12 years ago Closed 12 years ago

makefiles: services/sync - libs:: add deps so depend builds can be a nop

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: joey, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

Use generic rules, v1
2.92 KB, patch
glandium
: review+
Details | Diff | Splinter Review 
services/sync/Makefile.in
=====================================================

libs:: target - convert shell script logic into make deps & targets so dependency builds can become a nop.


Move find -type [d|f] logic outside libs:: and gather dirs/files into vars.
Use $(patsubst $(topsrcdir)) to form a relative path from $(srcdir).

Use $(call mkdir_deps,$(dirs)) to create dependencies, avoiding un-necessary calls to mkdir -p/nsinstall.py -D when directories exist.


Normal dependencies / target rules can handle copying files *only* when sources change.

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

libs-preqs =\
  $(call mkdir_deps,$(dirs)) \
  $(generate-a-list-of-services-deps-from files) \

libs: $(libs-preqs)




# Preprocess constants (by preprocessing everything).
# The 'HERE' idiom avoids a dependency on pushd. We need to do this fiddling in
# order to get relative paths, so we can process services/sync/modules/* into
# modules/services-sync/*.
# 
# Note that we find candidates, make directories, then 'copy' files.
libs::
ifndef NO_DIST_INSTALL
	$(EXIT_ON_ERROR) \
	HERE=$(CURDIR); \
	cd $(srcdir)/modules; \
	dirs=`find * -type d`; \
	files=`find * -type f`; \
	cd $$HERE; \
	for d in $$dirs; do \
		$(PYTHON) $(topsrcdir)/config/nsinstall.py -D $(FINAL_TARGET)/modules/services-sync/$$d; \
	done; \
	for i in $$files; do \
		src=$(srcdir)/modules/$$i; \
		dest=$(FINAL_TARGET)/modules/services-sync/$$i; \
		$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(SYNC_PP_DEFINES) $$src > $$dest ; \
	done
endif
Assignee: nobody → joey
Comment on attachment 628116 [details] [diff] [review]
services/sync/Makefile.in - add deps so depend build can become a nop

Rearranged libs:: target logic so deps can be used to control the build.

Gather files into local vars when processing the libs or clobber/clean target.
Use $(subst/patsubst) to generate relative paths in place of cd && find.

From collected paths derive directory dependencies (mkdir_deps).
Generate a list of (FINAL_FILES) in the destination directory.
Target rule added to use these paths, match based on path beneath sync/ and install sources into the FINAL_FILES directory.

Added a GARBAGE += entry so make clean can do it's thing.
Attachment #628116 - Flags: review?(khuey)
Comment on attachment 628116 [details] [diff] [review]
services/sync/Makefile.in - add deps so depend build can become a nop

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

::: services/sync/Makefile.in
@@ +30,5 @@
>  PREF_JS_EXPORTS = $(srcdir)/services-sync.js
>  
> +
> +# process only if gmake [libs|clobber]
> +ifneq (,$(call isTargetStem,libs,clean,clobber)) #{

This will fail to do anything when doing make -C objdir/services/sync.

@@ +32,5 @@
> +
> +# process only if gmake [libs|clobber]
> +ifneq (,$(call isTargetStem,libs,clean,clobber)) #{
> +
> +  $(errorIfEmpty,FINAL_TARGET)

You need a call, here. Or actually, nothing at all, because since that's before the config/rules.mk include, FINAL_TARGET is not set yet.

@@ +41,5 @@
> +    dirs-raw   := $(shell find $(src-modules) -type d)
> +    files-raw  := $(shell find $(src-modules) -type f)
> +
> +    dirs   = $(subst $(src-modules),$(NULL),$(dirs-raw))
> +    files  = $(subst $(src-modules),$(NULL),$(files-raw))

patsubst $(src-modules)/%,%

@@ +44,5 @@
> +    dirs   = $(subst $(src-modules),$(NULL),$(dirs-raw))
> +    files  = $(subst $(src-modules),$(NULL),$(files-raw))
> +
> +    # Generate directory deps
> +	dirs-final = $(addprefix $(FINAL_TARGET)/modules/services-sync/,$(dirs))

there are extra spaces at the beginning of this line.

@@ +51,5 @@
> +    # Generate file deps: copy modules to services-sync
> +    libs-preqs += $(foreach file,$(files),\
> +                      $(FINAL_TARGET)/modules/services-sync/$(file))
> +
> +    GARBAGE += $(libs-preqs)

We don't put files from FINAL_TARGET in GARBAGE.

@@ +63,5 @@
> +ifdef libs-preqs #{
> +
> +libs:: $(libs-preqs)
> +
> +# process services/sync/modules/* into modules/services-sync/*.

into $(FINAL_TARGET)/modules/services-sync/*..
Attachment #628116 - Flags: review?(khuey) → review-
You may want to see the marked duplicate bug for my recent attempts at this, including trying to create a generic preprocessing rule.
Attachment #628116 - Attachment is obsolete: true
Comment on attachment 628687 [details] [diff] [review]
services/sync/Makefile.in - add deps so depend build can become a nop

Try job: https://tbpl.mozilla.or/?tree=Try&rev=140505679f16

>> This will fail to do anything when doing make -C objdir/services/sync.

[sigh] Include of 'makeutils.mk' was omitted from the uploaded patch and that disabled the logic.  Re-added and a try job reports files were properly installed.  Permutations of make -C & cd obj/..../ make libs locally also function properly.

>> We don't put files from FINAL_TARGET in GARBAGE.

Replaced GARBAGE with a clean target able to remove $(libs-preqs) deps.  Clean should be able to remove any content a makefile is able to generate.  Otherwise "make clean all" can become a nop for verifying makefile edits.

>> into $(FINAL_TARGET)/modules/services-sync/*..

This was part of the original comment.  It was simply shortened to omit the shell specific content.
Attachment #628687 - Flags: review?(mh+mozilla)
Feel free to get rid of the $(shell find...) calls and enumerate files explicitly. If you don't want to do that, file a follow-up bug and I can take care of it.
(In reply to Gregory Szorc [:gps] from comment #6)
> You may want to see the marked duplicate bug for my recent attempts at this,
> including trying to create a generic preprocessing rule.

I've not touched services/crypto yet so that part of the patch would be unaffected.

ps> $(translate_files)
source_files := $(filter-out $translate_files, $(shell cd $(srcdir)/modules && find * -type f))
Comment on attachment 628687 [details] [diff] [review]
services/sync/Makefile.in - add deps so depend build can become a nop

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

::: services/sync/Makefile.in
@@ +11,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
> +USE_AUTOTARGETS_MK = 1
> +include $(topsrcdir)/config/makefiles/makeutils.mk
> +

Since you moved the targets after the inclusion of rules.mk, this shouldn't be needed.

@@ +47,5 @@
> +    dirs-raw   := $(shell find $(src-modules) -type d)
> +    files-raw  := $(shell find $(src-modules) -type f)
> +
> +    dirs   = $(subst $(src-modules),$(NULL),$(dirs-raw))
> +    files  = $(subst $(src-modules),$(NULL),$(files-raw))

patsubst $(src-modules)/%,%

@@ +76,5 @@
> +
> +
> +##
> +clean::
> +	$(RM) $(libs-preqs)

That has the same behavior as using GARBAGE. Files in FINAL_TARGET are cleaned up separately (and globally). Just remove this, since all targets are in FINAL_TARGET.

> Otherwise "make clean all" can become a nop for verifying makefile edits.

Adding Makefile.in as dep for the files in FINAL_TARGET sounds better, and will actually guarantee incremental builds on tinderbox work as expected.
Attachment #628687 - Flags: review?(mh+mozilla) → review-
Come to think of it, instead of the libs target depending on the directory, it should be each file target depending on the directory they are created in. (if we're going to do it the make way, let's do it right)
(In reply to Mike Hommey [:glandium] from comment #11)
> Comment on attachment 628687 [details] [diff] [review]
> services/sync/Makefile.in - add deps so depend build can become a nop
> 
> Review of attachment 628687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/Makefile.in
> @@ +11,5 @@
> >  include $(DEPTH)/config/autoconf.mk
> >  
> > +USE_AUTOTARGETS_MK = 1
> > +include $(topsrcdir)/config/makefiles/makeutils.mk
> > +
> 
> Since you moved the targets after the inclusion of rules.mk, this shouldn't
> be needed.
> 
> @@ +47,5 @@
> > +    dirs-raw   := $(shell find $(src-modules) -type d)
> > +    files-raw  := $(shell find $(src-modules) -type f)
> > +
> > +    dirs   = $(subst $(src-modules),$(NULL),$(dirs-raw))
> > +    files  = $(subst $(src-modules),$(NULL),$(files-raw))
> 
> patsubst $(src-modules)/%,%

$(src-modules) should be a unique string for a simple replacement correct ?  Path is anchored from / and length will not be a trivial string.


> @@ +76,5 @@
> > +
> > +
> > +##
> > +clean::
> > +	$(RM) $(libs-preqs)
> 
> That has the same behavior as using GARBAGE. Files in FINAL_TARGET are
> cleaned up separately (and globally). Just remove this, since all targets
> are in FINAL_TARGET.

Removing GARBAGE= or the clean target from this makefile and running 'make clean' interactively to test edits was not able to remove files that the makefile had installed beneath dist/.

If clean should remove these targets by default there may be a problem in rules.mk.  Otherwise GARBAGE= -or- clean will be needed to support local testing.

> > Otherwise "make clean all" can become a nop for verifying makefile edits.
> 
> Adding Makefile.in as dep for the files in FINAL_TARGET sounds better, and
> will actually guarantee incremental builds on tinderbox work as expected.

GLOBAL_DEPS can be added to help tinderbox builds but depending on Makefile.in for 'make clean' behavior will not provide adequate coverage.  While sitting in obj/ and modifying/testing edits within Makefile, Makefile.in will not come into play because target deps are newer.
(In reply to Mike Hommey [:glandium] from comment #12)
> Come to think of it, instead of the libs target depending on the directory,
> it should be each file target depending on the directory they are created
> in. (if we're going to do it the make way, let's do it right)

Hmmm that is a lot of extra file stats but if we go that route another bug can be opened to introduce those dependencies rather than forcing a rewrite of the target rules.
make clean in a subdirectory of the objdir has never been expected to remove files from $(DIST), and should not be expected to.
Attachment #628687 - Attachment is obsolete: true
Comment on attachment 628715 [details] [diff] [review]
services/sync/Makefile.in - add deps so depend build can become a nop

- removed makeutils.in
- included ifdef ENABLE_TESTS conditional around TEST_DIRS from gps's patch.
- place a dependency on GLOBAL_DEPS so makefile edits will force stale deps (~tinderbox dep builds).
- subst changed to patsubst - trailing slash not needed in the replacement, find will append a trailing slash.
- clean target retained so "make clean all" can verify installing files beneath obj/dist.

Another try job is in the pipeline for these edits.
Attachment #628715 - Flags: review?(mh+mozilla)
(In reply to Joey Armstrong [:joey] from comment #17)

> - included ifdef ENABLE_TESTS conditional around TEST_DIRS from gps's patch.

That ifdef should be removed. My patch was old and crufty.
Comment on attachment 628715 [details] [diff] [review]
services/sync/Makefile.in - add deps so depend build can become a nop

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

::: services/sync/Makefile.in
@@ +1,1 @@
> +# -*- makefile -*-

Do we really need that? In a file called Makefile.in?

@@ +29,5 @@
>    $(NULL)
>  
>  PREF_JS_EXPORTS = $(srcdir)/services-sync.js
>  
> +ifdef ENABLE_TESTS

This is not required. TEST_DIRS are only considered when ENABLE_TESTS is defined. (And this wasn't in previous iterations)

@@ +35,4 @@
>  endif
>  
> +include $(topsrcdir)/config/rules.mk
> +$(errorIfEmpty,FINAL_TARGET)

This is not necessary.

@@ +66,5 @@
> +##
> +ifdef libs-preqs #{
> +
> +libs:: $(libs-preqs)
> +$(libs-preqs): $(GLOBAL_DEPS)

I don't think the mkdir_deps should depend on GLOBAL_DEPS

@@ +77,5 @@
> +
> +
> +## Needed to remove deps installed beneath obj/dist/bin/modules
> +clean::
> +	$(RM) $(libs-preqs)

Again, please don't.
Attachment #628715 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 628715 [details] [diff] [review]
> services/sync/Makefile.in - add deps so depend build can become a nop
> 
> Review of attachment 628715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/Makefile.in
> @@ +1,1 @@
> > +# -*- makefile -*-
> 
> Do we really need that? In a file called Makefile.in?

It helps when copying the file to Makefile.bk, Makefile.orig, for colorization.
I'm not opposed to pulling it out, easy enough to add in when needed.

 
> @@ +29,5 @@
> >    $(NULL)
> >  
> >  PREF_JS_EXPORTS = $(srcdir)/services-sync.js
> >  
> > +ifdef ENABLE_TESTS
> 
> This is not required. TEST_DIRS are only considered when ENABLE_TESTS is
> defined. (And this wasn't in previous iterations)

True, propogated from gps's patch.  Removing to eliminate the delta.

> @@ +35,4 @@
> >  endif
> >  
> > +include $(topsrcdir)/config/rules.mk
> > +$(errorIfEmpty,FINAL_TARGET)
> 
> This is not necessary.

Removing but that will ignore cases where config.mk fails to load or FINAL_TARGET has lost or had value over-written.


> @@ +66,5 @@
> > +##
> > +ifdef libs-preqs #{
> > +
> > +libs:: $(libs-preqs)
> > +$(libs-preqs): $(GLOBAL_DEPS)
> 
> I don't think the mkdir_deps should depend on GLOBAL_DEPS
> 
> @@ +77,5 @@
> > +
> > +
> > +## Needed to remove deps installed beneath obj/dist/bin/modules
> > +clean::
> > +	$(RM) $(libs-preqs)
> 
> Again, please don't.

make clean *does-not-work-properly* w/o this target.

make clean will not remove files installed beneath obj/dist.  Trying to run "make clean; make" is effectively a nop because files cannot be removed then reinstalled.
Attachment #628715 - Attachment is obsolete: true
(In reply to Joey Armstrong [:joey] from comment #20)
> make clean will not remove files installed beneath obj/dist.

make clean does not remove files installed beneath obj/dist.
Removed cosmetic edits:
  o -*- makefile -*-
  o ifdef ENABLE_TESTS
  o errorIfEmpty

Clean target retained because local testing of makefile edits will be incomplete w/o it.  Make clean should be able to remove all of the content generated by a makefile.  Library garbage/clean targets are currently not able to do this.

Another try job is on the way.
(In reply to Mike Hommey [:glandium] from comment #22)
> (In reply to Joey Armstrong [:joey] from comment #20)
> > make clean will not remove files installed beneath obj/dist.
> 
> make clean does not remove files installed beneath obj/dist.

Why not ?  Clean targets in general should be able to remove any targets a makefile is able to generate { one exception: common directories }.  If these targets are not removed we have a hole in local testing because pre-existing targets can short-circuit exercising target logic that has been changed.
(In reply to Joey Armstrong [:joey] from comment #24)
> Why not ?

Because that's how our build system works.

Let's say we want to have make clean do it anyways. Then there's another problem: when you remove one of the files from the source directory, make clean is not going to remove the corresponding file in the install directory. Likewise if you change the Makefile to install to a different location. And so on.

So, there are too many side-problems that the patch doesn't address, and that are going to be similarly not addressed in other places where you would want to replace the sequential libs:: rules into make dependencies. So maybe you should start by making a rules.mk helper for file installation under $DIST, and that would deal with everything. And the solution to most problems might be to track which files were installed where, so that make clean can remove them, and, even better an incremental make can remove the extra files too. (OTOH, there might be interesting problems when files move from one source dir to another, but are still installed to the same place)
(In reply to Mike Hommey [:glandium] from comment #25)
> (In reply to Joey Armstrong [:joey] from comment #24)
> > Why not ?
> 
> Because that's how our build system works.

I would assert this assumption is broken and should be fixed.

Makfiles having the ability to remove content they are able to generate is a fundamental activity for writing them.  W/o this ability make cannot simply 'do the right thing' later on.

Testing becomes a manual process that {very bad imho} people need to be aware so they can search out and remove all the little creepie-crawlies in order to properly test changes that are being made.

For this makefile/patch specifically there is no reason testing cannot be automated.  Clean simply should do what it is expected to do, remove deps that were installed by this target so a test can verify they were generated properly.


The only reason for not doing this that I can think of is breaking behavior for another makefile/directory.  But for something like this to happen it would imply that either the clean target for a makefile was broken {removing targets s makefile was not able to generate} or worse circular dependencies exist that updated timestamps would later trip over.

Either of these are a problem set that should be addressed IMHO.  Esp circular deps within the build as they alone cause a whole host of fun problems.



> Let's say we want to have make clean do it anyways. Then there's another
> problem: when you remove one of the files from the source directory, make
> clean is not going to remove the corresponding file in the install
> directory. Likewise if you change the Makefile to install to a different
> location. And so on.

Why would this be a problem specific to an extra local clean target ?  We have the same general problem now which is usually addressed by performing a clobber build.  Blow away the obj directory, dist/, etc to purge the stray/deleted files then rebuild.

We could add extra meta and track sources/directories added and removed so clean could be smarter about removal but are we not using clobber builds as a general answer to that problem ?

Source deletions would probably be easy to handle given all the hardcoded paths, just maintain an exclusion list.  Source deletions become a 2 step process, first submit a makefile edit with sources added on the exclusion list.  Then sometime later after the deletion has landed go back and purge the exclusion list.


> So, there are too many side-problems that the patch doesn't address, and
> that are going to be similarly not addressed in other places where you would
> want to replace the sequential libs:: rules into make dependencies. So maybe
> you should start by making a rules.mk helper for file installation under
> $DIST, and that would deal with everything. And the solution to most
> problems might be to track which files were installed where, so that make
> clean can remove them, and, even better an incremental make can remove the
> extra files too. (OTOH, there might be interesting problems when files move
> from one source dir to another, but are still installed to the same place)

Yes fixing this problem for the general case would be preferable but that answer requires far more infrastructure and effort to try and deploy.  In the interim people still have to deal with a problem that adversely affects testing and local use.

I would assert that we start small with bridge building and begin fixing these pain points locally to gain improvement in the system.  Then later on when the general use case answer is ready for deployment { hopefully based on all of the smaller/local/interim answers }.  Revisit the local patches and dismantle the bridges when they are no longer needed.

Fix pain points and system improvements can be worked on while the longer term general answers are being worked on.
I bitrotted this in bug 774736. Changes are mostly cosmetic, however.

It doesn't matter because the current patch has $(shell) calls, which violate bug 769390;

Also, the preprocessor now warns when there are no substitutions. Most of Sync's files don't have substitutions. We should only invoke the preprocessor on the file(s) that need it.
Blocks: nomakerules
Attached patch Add file lists to Makefile (obsolete) — — Splinter Review 
This patch doesn't work.

I added lists of modules to the Makefile.in and split up the rules so it is pretty obvious what should go on. This doesn't work because some nsinstall args or something aren't proper.

But, joey or somebody should be able to figure out what's going on and do the right thing.

Per our goal of no make rules, we may need a new magic rule in rules.mk that installs JS into the proper destination directory. Maybe this can be done with the definition of a new variable. I dunno. I'll leave it for joey to decide.
Depends on: 775338
Refactored to use newly-committed generic preprocessor and install rules.
Assignee: joey → gps
Attachment #628749 - Attachment is obsolete: true
Attachment #643126 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #650223 - Flags: review?(mh+mozilla)
I updated my local patch to include config.mk instead of both config.mk and autoconf.mk after remembering that config.mk includes autoconf.mk. I'll save everyone the bug spam.
Comment on attachment 650223 [details] [diff] [review]
Use generic rules, v1

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

::: services/sync/Makefile.in
@@ +23,4 @@
>   -Dweave_version=$(weave_version) \
>   -Dweave_channel=$(weave_channel) \
>   -Dweave_id=$(weave_id)
> +SYNC_PP_PATH := $(FINAL_TARGET)/modules/services-sync

If you make it a =, you don't need config.mk. Either way works for me.

@@ +68,5 @@
>    $(NULL)
>  
>  PREF_JS_EXPORTS := $(srcdir)/services-sync.js
>  
> +install_path += $(FINAL_TARGET)/modules/services-sync

= instead of +=

@@ +73,3 @@
>  
> +# Install JS module files.
> +SYNC_MAIN_FILES := $(foreach module,$(sync_modules),modules/$(module))

$(addprefix modules/,$(sync_modules))

@@ +78,2 @@
>  
> +SYNC_ENGINES_FILES := $(foreach module,$(sync_engine_modules),modules/engines/$(module))

likewise.
Attachment #650223 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/services/services-central/rev/af422959e662
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/af422959e662
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: