Closed
Bug 680246
Opened 13 years ago
Closed 12 years ago
makefiles: add a makefile library rule to support thread safe directory creation
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: joey, Assigned: joey)
References
Details
Attachments
(1 file, 12 obsolete files)
25.23 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Add a target in rules.mk to create directories in a thread safe manner and controlled by dependency. rules.mk::$(XPIDL_GEN_DIR)/.done could be generalized. Unit test makefiles are starting to accumulate duplicate directory creation logic so it is probably time to move this into a library. Also derived targets may allow replacing the asynchronous mkdir calls: run_for_side_effects = $(shell mkdir...) with real dependencies
Assignee | ||
Comment 1•13 years ago
|
||
DIR_DEPS - new macro logic will pay attention to. AUTO_DEPS - a list of generated dependencies for all: to use $(dir)/.dir.done - timestamp dependency file that can be used by individual targets. $(dir)/.done was not used to avoid potential ambiguity in a library target. build/unix/test/Makefile.in - unit test updated to use the library target.
Comment on attachment 554208 [details] [diff] [review] macros and targets supporting thread safe mkdir w/deps Some comments: I think that we should automatically add the directory to GARBAGE_DIRS. AUTO_DEPS should probably be some sort of macro or generated variable that takes a directory name. DIR_DEPS doesn't really explain what it's doing. How about GENERATED_DIRS? Ted may have a better suggestion. I imagine it looking something like GENERATED_DIRS += _xpidlgen $(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(_xpidlgen_DIR_DEP) commands or GENERATED_DIRS += _xpidlgen $(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(call GENERATED_DIR_DEP,_xpidlgen) commands
Attachment #554208 -
Flags: review?(khuey)
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2) > Comment on attachment 554208 [details] [diff] [review] > macros and targets supporting thread safe mkdir w/deps > > Some comments: > > I think that we should automatically add the directory to GARBAGE_DIRS. This logic was present in an early version but a problem surfaced. If someone were to place a dependency on a common/shared directory not exclusively maintained by the makefile (ex: dist/include) -- "make clean" would then be able to remove a lot more targets than a single directory/makefile would be able to regenerate. > AUTO_DEPS should probably be some sort of macro or generated variable that > takes a directory name. AUTO_DEPS - I have a longer term usage in mind for this macro. A function could be added to encapsulate the actual generation logic so values are explicitly derived by path { $(foreach dir,$(call foo $dir),$(GENERATED_DIRS)). But I opted for a more generic name in this case so it would be trivial to conditionally add more targets later on w/o having to explicitly modify makefiles in the sandbox. > DIR_DEPS doesn't really explain what it's doing. How about GENERATED_DIRS? > Ted may have a better suggestion. That seems reasonable given naming will become more intuitive. > I imagine it looking something like > > GENERATED_DIRS += _xpidlgen > > $(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(_xpidlgen_DIR_DEP) > commands > > or > > GENERATED_DIRS += _xpidlgen > > $(XPIDL_GEN_DIR)/%.h: %.idl $(XPIDL_DEPS) $(call GENERATED_DIR_DEP,_xpidlgen) > commands Yes exactly
(In reply to Joey Armstrong [:joey] from comment #3) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2) > > Comment on attachment 554208 [details] [diff] [review] > > macros and targets supporting thread safe mkdir w/deps > > > > Some comments: > > > > I think that we should automatically add the directory to GARBAGE_DIRS. > > This logic was present in an early version but a problem surfaced. If > someone were to place a dependency on a common/shared directory not > exclusively maintained by the makefile (ex: dist/include) -- "make clean" > would then be able to remove a lot more targets than a single > directory/makefile would be able to regenerate. Yeah, that's a good point.
Assignee | ||
Comment 5•13 years ago
|
||
Patch includes Kyle's suggestions. Renamed DIR_DEPS to GENERATED_DIRS Added factory macros for dependency generation. User funcs added to help make the code self-documenting. .SECONDEXPANSION used to allow generating prereqs from the current target.
Attachment #554208 -
Attachment is obsolete: true
Attachment #555810 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 6•13 years ago
|
||
same mkdir library logic but replaced '/bin/true' refs with 'true' so mac [/usr/bin/true] can find the command.
Attachment #558599 -
Flags: review?(coop)
Comment 7•13 years ago
|
||
Comment on attachment 558599 [details] [diff] [review] threadsafe mkdir + cmd path fix for mac Review of attachment 558599 [details] [diff] [review]: ----------------------------------------------------------------- Some indentation questions, but otherwise looks fine to me. ::: config/makefiles/autotargets.mk @@ +42,5 @@ > + > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) > + > + threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir))) Is the indentation correct on lines 43 and 46? Is it just to make the = line up? ::: js/src/config/makefiles/autotargets.mk @@ +42,5 @@ > + > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) > + > + threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir))) Same comment about indentation here.
Attachment #558599 -
Flags: review?(coop) → review+
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Chris Cooper [:coop] from comment #7) > Comment on attachment 558599 [details] [diff] [review] > threadsafe mkdir + cmd path fix for mac > > Review of attachment 558599 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some indentation questions, but otherwise looks fine to me. > > ::: config/makefiles/autotargets.mk > @@ +42,5 @@ > > + > > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) > > + > > + threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir))) > > Is the indentation correct on lines 43 and 46? Is it just to make the = line > up? > > ::: js/src/config/makefiles/autotargets.mk > @@ +42,5 @@ > > + > > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) > > + > > + threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir))) > > Same comment about indentation here. Yes some of the lines are odd sizes. Like macros are alinged on the '=' to limit scanning focus while reading through the logic.
Comment 9•13 years ago
|
||
Just FWIW, coop's not a build system peer, so his review isn't sufficient to land this.
Updated•13 years ago
|
Attachment #555810 -
Attachment is obsolete: true
Attachment #555810 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•13 years ago
|
Attachment #558599 -
Flags: review+ → review?(ted.mielczarek)
Assignee | ||
Comment 10•13 years ago
|
||
ping on pending code review from the 7th
Comment 11•13 years ago
|
||
Comment on attachment 558599 [details] [diff] [review] threadsafe mkdir + cmd path fix for mac Review of attachment 558599 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, sorry again for the delay in review. I like the concept a lot, I just have some quibbles with the implementation, and a few style nits. I'm r-'ing solely because I want you to remove the intermediate dependencies as discussed below, since I think that will result in a fairly large change here. The rest of it is fairly minor stuff. ::: build/unix/test/Makefile.in @@ +44,5 @@ > include $(DEPTH)/config/autoconf.mk > + > +TS = .ts > +GENERATED_DIRS += $(TS) > + GARBAGE_DIRS += $(TS) I believe that our style here is usually more like: GENERATED_DIRS += $(TS) GARBAGE_DIRS += $(TS) that is, line up the operators, but keep variable names left-aligned. (Not that we have a Makefile style guide anywhere, we really should.) ::: config/makefiles/autotargets.mk @@ +43,5 @@ > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) > + > + threadsafe_mkdir = $(foreach dir,$(1),$(call gen_auto_macro,threadsafe_mkdir,$(dir))) > +gen_threadsafe_deps = $(addsuffix /.dir.done,$(strip $(call get_auto_arg,$(1),1))) As we discussed on IRC, I don't think the intermediate ^threadsafe_mkdir deps here provide any extra value. You've already got the .dir.done targets that serialize the mkdir, and these add a bit of complexity that we could drop. I'd rather that you set AUTO_DEPS = $(foreach dir,$(GENERATED_DIRS),$(dir)/.dir.done), and just use the rules below to generate those. If you'd like the targets to be more self-descriptive, then maybe rename .dir.done to .threadsafe.mkdir.done or something. @@ +55,5 @@ > +## all bootstrap: $(AUTO_DEPS) > +## target: $(dir)/.dir.done $(dir)/foobar > +########################################################################### > +ifdef GENERATED_DIRS > + AUTO_DEPS += $(foreach dir,$(GENERATED_DIRS),$(call threadsafe_mkdir,$(GENERATED_DIRS))) As discussed on IRC, this should be calling threadsafe_mkdir on $(dir), since you're also looping in threadsafe_mkdir above. @@ +58,5 @@ > +ifdef GENERATED_DIRS > + AUTO_DEPS += $(foreach dir,$(GENERATED_DIRS),$(call threadsafe_mkdir,$(GENERATED_DIRS))) > +endif > + > +.SECONDARY: # preserve intermediates: .dir.done Doesn't feel right to enable this globally. Can you do .SECONDARY: $(AUTO_DEPS) or something instead? (Assuming you've made the change I asked for above.) @@ +59,5 @@ > + AUTO_DEPS += $(foreach dir,$(GENERATED_DIRS),$(call threadsafe_mkdir,$(GENERATED_DIRS))) > +endif > + > +.SECONDARY: # preserve intermediates: .dir.done > +.SECONDEXPANSION: # Enable for prereq deps derived from target/$@ If you drop the ^threadsafe_mkdir bit then you don't need the SECONDEXPANSION, right? @@ +71,5 @@ > +## - macro AUTO_DEPS can be used to explicitly add a list of deps > +## - single: $(call threadsafe_mkdir,$(var)) > +## - $(foreach dir,$(list),$(call threadsafe_mkdir,$(dir))) > +################################################################### > +%/: %/.dir.done I think I'd feel better with this as a static pattern rule, so we only match on exactly what we mean, probably like: $(GENERATED_DIRS): %: %/.dir.done
Attachment #558599 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 12•13 years ago
|
||
Removed the macro dep/string generation layer, nit edits as requested. Edits verified by: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=37bcb0d8c33a
Attachment #558599 -
Attachment is obsolete: true
Attachment #561883 -
Flags: review?(ted.mielczarek)
Comment 13•13 years ago
|
||
Comment on attachment 561883 [details] [diff] [review] mkdir -p patch with macro dep generators removed. Review of attachment 561883 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Just some real minor fiddly comments. ::: config/makefiles/autotargets.mk @@ +40,5 @@ > + > +SPACE ?= $(NULL) $(NULL) > + > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) Doesn't look like you're using these anymore. @@ +52,5 @@ > +## all bootstrap: $(AUTO_DEPS) > +## target: $(dir)/.dir.done $(dir)/foobar > +########################################################################### > +ifdef GENERATED_DIRS > + GENERATED_DIRS_DEP = $(foreach dir,$(GENERATED_DIRS),$(dir)/.dir.done) Maybe name this GENERATED_DIRS_DEPS (plural)? @@ +64,5 @@ > +## - targets suffixed by a slash will match and be processed > +## - macro AUTO_DEPS can be used to explicitly add a list of deps > +## - single: $(call threadsafe_mkdir,$(var)) > +## - $(foreach dir,$(list),$(call threadsafe_mkdir,$(dir))) > +################################################################### This comment looks like it's out of sync with what you're actually doing now. @@ +65,5 @@ > +## - macro AUTO_DEPS can be used to explicitly add a list of deps > +## - single: $(call threadsafe_mkdir,$(var)) > +## - $(foreach dir,$(list),$(call threadsafe_mkdir,$(dir))) > +################################################################### > +$(GENERATED_DIRS_DEP): Oh huh, this winds up even simpler than I suggested. Nice!
Attachment #561883 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to Ted Mielczarek [:ted, :luser] from comment #13) > Comment on attachment 561883 [details] [diff] [review] [diff] [details] [review] > mkdir -p patch with macro dep generators removed. > > Review of attachment 561883 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: config/makefiles/autotargets.mk > @@ +40,5 @@ > > + > > +SPACE ?= $(NULL) $(NULL) > > + > > + get_auto_arg = $(word $(2),$(subst ^,$(SPACE),$(1))) # get(1=var, 2=offset) > > +gen_auto_macro = $(addsuffix ^$(1),$(2)) # gen(1=target_pattern, 2=value) > > Doesn't look like you're using these anymore. I ended up leaving these two in because they have potential downstream use as library functions. All of the other nits to be fixed shortly. Thanks -- Joey
Assignee | ||
Comment 15•13 years ago
|
||
nit fixes for the last code review
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 16•13 years ago
|
||
The try run in comment 12 was orange on Win opt: { make[1]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/browser/locales' sed: can't read components/*.manifest: No such file or directory make[4]: *** [repackage-zip] Error 2 make[3]: *** [repackage-win32-installer] Error 2 make[2]: *** [repackage-win32-installer-x-test] Error 2 make[1]: *** [l10n-check] Error 2 make: *** [l10n-check] Error 2 } When you attach the updated patch, could you add a commit message to it, as I probably won't be able to summarise the changes as accurately as you could. Thanks :-)
Keywords: checkin-needed
Assignee | ||
Comment 17•13 years ago
|
||
nit fixes + checkin messages
Attachment #562030 -
Attachment is obsolete: true
Assignee | ||
Comment 18•13 years ago
|
||
(In reply to Ed Morley [:edmorley] from comment #16) > The try run in comment 12 was orange on Win opt: > { > make[1]: Leaving directory > `/e/builds/moz2_slave/try-w32/build/obj-firefox/browser/locales' > sed: can't read components/*.manifest: No such file or directory > make[4]: *** [repackage-zip] Error 2 > make[3]: *** [repackage-win32-installer] Error 2 > make[2]: *** [repackage-win32-installer-x-test] Error 2 > make[1]: *** [l10n-check] Error 2 > make: *** [l10n-check] Error 2 > } > > When you attach the updated patch, could you add a commit message to it, as > I probably won't be able to summarise the changes as accurately as you > could. Thanks :-) New patch submitted w/checkin message. A missing manifest file should be unrelated to the code changes, makefile rule not yet used in the sandbox and a unit test update. Similar manifest problems are also showing for bug #515374/Packager.pm/try submissions so there may be a bug lurking in the shadows. I'll open another after the source has been isolated.
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 19•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/020f14584a2a
Target Milestone: --- → mozilla9
Updated•13 years ago
|
Keywords: checkin-needed
Comment 20•13 years ago
|
||
This broke the build across all platforms, so I backed it out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=020f14584a2a https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9c0a2a1a25
Target Milestone: mozilla9 → ---
The copy-paste test strikes again!
Assignee | ||
Comment 22•13 years ago
|
||
gotta love code duplication...
Assignee | ||
Comment 23•13 years ago
|
||
Two edits: o function mkdir_deps declared so caller can generate dependencies directly in place of using $(AUTO_DEPS) o renamed the .dir.done dep to .mkdir.done to reduce the chance of conflicts https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4822254e25e3 o linuxqt - segv in unit test - bug 673017 o win32 - make package error - 684095 o win64 - bug 690337 o linux-mobile - bug 690198 dpkg-architecture not found o linux(64)?, macosx(64)?, win32 - bug 684095 - JS Component Loader ERROR during packaging
Attachment #561883 -
Attachment is obsolete: true
Attachment #562161 -
Attachment is obsolete: true
Attachment #563398 -
Flags: review?(ted.mielczarek)
Updated•13 years ago
|
Attachment #563398 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 24•13 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/cb715f8a1363
Keywords: checkin-needed
Whiteboard: fixed-in-bs
https://hg.mozilla.org/mozilla-central/rev/cb715f8a1363
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Comment 26•13 years ago
|
||
Backed out: https://hg.mozilla.org/mozilla-central/rev/f25928e4847d because of Win opt package errors: https://tbpl.mozilla.org/php/getParsedLog.php?id=6632240&tree=Firefox (The same errors were seen on the original pushes to Try and to b-s.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-bs
Target Milestone: mozilla10 → ---
Assignee | ||
Comment 27•13 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #26) > Backed out: https://hg.mozilla.org/mozilla-central/rev/f25928e4847d > > because of Win opt package errors: > https://tbpl.mozilla.org/php/getParsedLog.php?id=6632240&tree=Firefox > > (The same errors were seen on the original pushes to Try and to b-s.) Packaging errors have absolutely nothing to do with this patch. Target rules were added to create directories and that logic is currently used by one makefile/unit test: build/unix/test/Makefile.
Assignee | ||
Comment 28•13 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #26) > Backed out: https://hg.mozilla.org/mozilla-central/rev/f25928e4847d > > because of Win opt package errors: > https://tbpl.mozilla.org/php/getParsedLog.php?id=6632240&tree=Firefox > > (The same errors were seen on the original pushes to Try and to b-s.) Here is the bug for l10n/packager error that was tickled by the submission: https://bugzilla.mozilla.org/show_bug.cgi?id=690138
Assignee | ||
Comment 29•13 years ago
|
||
w32-ix-slave37: abort: There is no Mercurial repository here.
Comment 30•13 years ago
|
||
Regardless whether the bug is in this patch or elsewhere, it appears to be triggered or exposed by this patch. This landing turned m-c and b-s permaorange until it was backed out, so bug 690138 will need to be fixed before this can re-land.
Blocks: 690138
Comment 31•13 years ago
|
||
(In reply to Joey Armstrong from comment #6) > replaced '/bin/true' refs with 'true' so mac [/usr/bin/true] can find the command. Or indeed the builtin true will get used in those shells that support it.
Assignee | ||
Comment 32•12 years ago
|
||
Patch refresh. Make logic roughly the same but logic moved outside of rules.mk for modularity and isolation. Unit test rewritten in python. Test driver will setup a scratch work area then pass arguments to a template makefile to invoke tests. Copyright block in new files has been replaced with the latest MPL boilerplate. Patch submitted for review only. Try submissions have logged several unrelated failures that will need to be triaged before final submission.
Attachment #563398 -
Attachment is obsolete: true
Attachment #600129 -
Flags: review?(ted.mielczarek)
Comment 33•12 years ago
|
||
Comment on attachment 600129 [details] [diff] [review] make library rule for threadsafe mkdir + unit tests New code should be under MPL 2.0. https://www.mozilla.org/MPL/license-policy.html
Assignee | ||
Comment 34•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #33) > Comment on attachment 600129 [details] [diff] [review] > make library rule for threadsafe mkdir + unit tests > > New code should be under MPL 2.0. > https://www.mozilla.org/MPL/license-policy.html heh updating the mdn coding style page. The boilerplate link is referencing the 1.1 flavor.
Assignee | ||
Comment 35•12 years ago
|
||
Same patch as last upload but MPL text replaced with the 2.0 flavor. Also unit test is now dependent on makefiles/autotargets.mk rather than rules.mk.
Attachment #600156 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Summary: makefiles: add a generic rule.mk to support thread safe directory creation → makefiles: add a makefile library rule to support thread safe directory creation
Comment 36•12 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #30) > Regardless whether the bug is in this patch or elsewhere, it appears to be > triggered or exposed by this patch. This landing turned m-c and b-s > permaorange until it was backed out, so bug 690138 will need to be fixed > before this can re-land. :mbrubeck, after reading both bugs, I'm confused. Did you mean for bug#680246 to depend on bug#690138? (not the opposite?)
Comment 37•12 years ago
|
||
(In reply to John O'Duinn [:joduinn] from comment #36) > :mbrubeck, after reading both bugs, I'm confused. Did you mean for > bug#680246 to depend on bug#690138? (not the opposite?) Yeah, I got the dependency backward. Fixed now.
Assignee | ||
Comment 38•12 years ago
|
||
replacement patch pending with edits, cancelling the current review. Another patch should be uploaded today post reviewing the last batch of try logs for critical failure points.
Assignee | ||
Updated•12 years ago
|
Attachment #600156 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Attachment #600129 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 39•12 years ago
|
||
library makefile rules to automate directory in a threadsafe manner. Creation: GENERATED_DIRS += path1 path2 path3 deps = $(call mkdir_deps,macro_of_dirs) A unit test and makefile template included to validate creation by either macro or user function.
Attachment #600129 -
Attachment is obsolete: true
Attachment #600156 -
Attachment is obsolete: true
Attachment #603524 -
Flags: review?(khuey)
Comment 40•12 years ago
|
||
Comment on attachment 603524 [details] [diff] [review] threadsafe mkdir patch with unit test It looks like you took the MPL 1.1 license block and changed "1.1" to "2.0." This is incorrect. See https://www.mozilla.org/MPL/headers/ for the correct license block. Yes, it is only 3 lines!
Updated•12 years ago
|
Attachment #603524 -
Attachment is patch: true
Assignee | ||
Comment 41•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #40) > Comment on attachment 603524 [details] [diff] [review] > threadsafe mkdir patch with unit test > > It looks like you took the MPL 1.1 license block and changed "1.1" to "2.0." > This is incorrect. See https://www.mozilla.org/MPL/headers/ for the correct > license block. Yes, it is only 3 lines! So all of the disclaimer and contributor verbage should be replaced with the 3 line licence block ?
Assignee | ||
Comment 42•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #40) > Comment on attachment 603524 [details] [diff] [review] > threadsafe mkdir patch with unit test > > It looks like you took the MPL 1.1 license block and changed "1.1" to "2.0." > This is incorrect. See https://www.mozilla.org/MPL/headers/ for the correct > license block. Yes, it is only 3 lines! Also should the copyright notice within existing files be updated to the 2.0 flavor or should the text remain the same but change the end date to 2012 ?
Assignee | ||
Comment 43•12 years ago
|
||
edit_1.1: licence block for new files pruned to 3 lines edit_1.1: end date changed to 2012 in files with an existing license block. threadsafe mkdir patch with unit test library makefile rules to automate directory in a threadsafe manner. Creation: GENERATED_DIRS += path1 path2 path3 deps = $(call mkdir_deps,macro_of_dirs) A unit test and makefile template included to validate creation by either macro or user function.
Attachment #603524 -
Attachment is obsolete: true
Attachment #603524 -
Flags: review?(khuey)
Attachment #603675 -
Flags: review?(khuey)
Comment on attachment 603675 [details] [diff] [review] threadsafe mkdir patch with unit test Review of attachment 603675 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments addressed (either the changes made, or reasons why I'm wrong given). ::: config/makefiles/autotargets.mk @@ +35,5 @@ > +########################################################################### > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done) > + > +ifneq (,$(GENERATED_DIRS)) > + tmpauto :=$(call mkdir_deps,GENERATED_DIRS) There's a missing space after := Is there a compelling reason to do $($(1)) and GENERATED_DIRS, instead of $(1) and $(GENERATED_DIRS)? I'm not opposed to the way it is, just curious. @@ +41,5 @@ > + GARBAGE_DIRS +=$(tmpauto) > +endif > + > +%/.mkdir.done: > + $(subst $(SPACE)-p,$(null),$(MKDIR)) -p $(dir $@) I assume this is just to avoid 'mkdir -p -p dir'? Can you add a comment for that? Also, use $(NULL) with all caps. ::: config/rules.mk @@ +2039,5 @@ > +############################################################################# > +# Derived targets and dependencies > + > +include $(topsrcdir)/config/makefiles/autotargets.mk > +all:: $(AUTO_DEPS) Don't you want 'default all::'?
Attachment #603675 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 45•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44) > Comment on attachment 603675 [details] [diff] [review] > threadsafe mkdir patch with unit test > > Review of attachment 603675 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with the comments addressed (either the changes made, or reasons why > I'm wrong given). > > ::: config/makefiles/autotargets.mk > @@ +35,5 @@ > > +########################################################################### > > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done) > > + > > +ifneq (,$(GENERATED_DIRS)) > > + tmpauto :=$(call mkdir_deps,GENERATED_DIRS) > > There's a missing space after := Actually this syntax was intentional, maybe not specifically for this assignment but I am starting to use it in general. Removing whitespace from the left side of values will help prevent whitespace from accumulating within displayed commands/arguments: $cmd -a -debug ... > Is there a compelling reason to do $($(1)) and GENERATED_DIRS, instead of > $(1) and $(GENERATED_DIRS)? I'm not opposed to the way it is, just curious. GENERATED_DIRS in the function could be considered a form of hardcoding depending on your point of view. But there is a performance ding -- if it were listed, extra deps to check are introduced with each call. Calling mkdir_deps from a library function would re-add all directory deps explicitly defined by the calling makefile. Also $(1) will not easily allow for directory creation by list, this call would create one dir and silently fail on the rest: $(call mkdir_deps,foo bar tans fans) $($(1)) is able support creation by list: prereq = foo bar tans fans $(call mkdir_deps,prereq) Hmmmm, recently I wrote some utility macros that can identify argument context as either list or scalar and expand args as needed [1]. The functions could be included as an enhancement so the library can support both calling syntax: $(call mkdir_deps,a) $(call mkdir_deps,a b c d) [1] - anyone know of syntax similar to $* for make functions that would allow accessing all elements in the parameter list. Aside from using a hardcoded list: $(1) $(2) $(3) ... $(9) that would drop elements beyond $(10)+ ? > @@ +41,5 @@ > > + GARBAGE_DIRS +=$(tmpauto) > > +endif > > + > > +%/.mkdir.done: > > + $(subst $(SPACE)-p,$(null),$(MKDIR)) -p $(dir $@) > > I assume this is just to avoid 'mkdir -p -p dir'? Can you add a comment for > that? > > Also, use $(NULL) with all caps. Yes and I really need to sit down and write a make-lint checker for $(NULL)... Question: since null= can be used w/o pre-definition would this help re-define it as a builtin value ? Syntax will be changed for the bug but I have never really thought of null as a constant taken in this context. > ::: config/rules.mk > @@ +2039,5 @@ > > +############################################################################# > > +# Derived targets and dependencies > > + > > +include $(topsrcdir)/config/makefiles/autotargets.mk > > +all:: $(AUTO_DEPS) > > Don't you want 'default all::'? This is question not statement... I normally know when targets will be processed and try to avoid un-necessary deps. If 'default' were added would this force make to re-check all of the directory deps beyond the initial call that should create them ? When needed deps can be explicitly mentioned by targets in the handful of instances where needed.
Comment 46•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #45) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44) > > Comment on attachment 603675 [details] [diff] [review] > > threadsafe mkdir patch with unit test > > > > Review of attachment 603675 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > r=me with the comments addressed (either the changes made, or reasons why > > I'm wrong given). > > > > ::: config/makefiles/autotargets.mk > > @@ +35,5 @@ > > > +########################################################################### > > > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done) > > > + > > > +ifneq (,$(GENERATED_DIRS)) > > > + tmpauto :=$(call mkdir_deps,GENERATED_DIRS) > > > > There's a missing space after := > > Actually this syntax was intentional, maybe not specifically for this > assignment but I am starting to use it in general. Removing whitespace from > the left side of values will help prevent whitespace from accumulating > within displayed commands/arguments: > > $cmd -a -debug ... That kind of thing happens when you do $cmd $(var1) $(var2). That var1 and var2 are defined with var=something instead of var= something changes nothing.
Assignee | ||
Comment 47•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #46) > (In reply to Joey Armstrong [:joey] from comment #45) > > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44) > > > Comment on attachment 603675 [details] [diff] [review] > > > threadsafe mkdir patch with unit test > > > > > > Review of attachment 603675 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > r=me with the comments addressed (either the changes made, or reasons why > > > I'm wrong given). > > > > > > ::: config/makefiles/autotargets.mk > > > @@ +35,5 @@ > > > > +########################################################################### > > > > +mkdir_deps =$(foreach dir,$($(1)),$(dir)/.mkdir.done) > > > > + > > > > +ifneq (,$(GENERATED_DIRS)) > > > > + tmpauto :=$(call mkdir_deps,GENERATED_DIRS) > > > > > > There's a missing space after := > > > > Actually this syntax was intentional, maybe not specifically for this > > assignment but I am starting to use it in general. Removing whitespace from > > the left side of values will help prevent whitespace from accumulating > > within displayed commands/arguments: > > > > $cmd -a -debug ... > > That kind of thing happens when you do $cmd $(var1) $(var2). > That var1 and var2 are defined with > var=something > instead of > var= something > changes nothing. This behavior can be generated using only: $(cmd) $(cmd_args) blah it happens in variable construction not in the command line
Comment 48•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #47) > This behavior can be generated using only: > $(cmd) $(cmd_args) blah > > it happens in variable construction not in the command line a:=a b:= b cmd_args:=$(a) $(b) foo: $(cmd) $(cmd_args) blah The above won't add any extra space because b has a space between = and the value. It will have double space if cmd_args is empty.
Assignee | ||
Comment 49•12 years ago
|
||
Updated•12 years ago
|
Attachment #603675 -
Attachment is obsolete: true
Comment 50•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #41) > So all of the disclaimer and contributor verbage should be replaced with the > 3 line licence block ? Yes, the new licence header should replace this entire block: http://www.mozilla.org/MPL/headers/ eg: http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/Makefile.in http://mxr.mozilla.org/mozilla-central/source/b2g/components/b2g.idl (In reply to Joey Armstrong [:joey] from comment #42) > Also should the copyright notice within existing files be updated to the 2.0 > flavor or should the text remain the same but change the end date to 2012 ? Old files can be transitioned to the new format too (and will be mass changed by gerv soon anyway).
Assignee | ||
Comment 51•12 years ago
|
||
r=me, Kyle's review + nit fixes. edit_1.2 - mkdir -p -p commented & s/null/NULL edit_1.2 - compelling reason to do $($(1)) rather than $(1) ? - bug 734121 will add helpers so func args can process as either scalar, list or ref. Arg handling is currently by list ref/var expansion $($(1)). edit_1.2 - Don't you want 'default all::'? Added target along with export, libs and tools. edit_1.2 - remaining copyright blocks crushed down to the 3 line default.
Attachment #604095 -
Attachment is obsolete: true
Attachment #605402 -
Flags: review?(khuey)
Assignee | ||
Comment 52•12 years ago
|
||
Try submission status for the edits ==================================== Thanks for your try submission (http://hg.mozilla.org/try/pushloghtml?changeset=7d7450575f7d). It's the best! Watch https://tbpl.mozilla.org/?tree=Try&rev=7d7450575f7d for your results to come in. Builds and logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jarmstrong@mozilla.com-7d7450575f7d. This directory won't be created until the first builds are uploaded, so please be patient. ========================================================================== Job passed with the usual set of messages: can't read dom/tests/mochitest/glboalstorage/Makefile.in: No such file or dir configure: WARNING: unrecognized options: --enable-debug, --enable-application tar: image/test/reftest/encoders-lossless/encoder.html?img=size-15x15.png&mime=image: Cannot stat: No such file or directory.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 53•12 years ago
|
||
ping on the code review
Attachment #605402 -
Flags: review?(khuey) → review+
Comment 54•12 years ago
|
||
Comment on attachment 605402 [details] [diff] [review] cary review forward with req cleanups Review of attachment 605402 [details] [diff] [review]: ----------------------------------------------------------------- ::: allmakefiles.sh @@ +1,5 @@ > #! /bin/sh > # > +# 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/. Please don't do that. http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/
Assignee | ||
Comment 55•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #54) > Comment on attachment 605402 [details] [diff] [review] > cary review forward with req cleanups > > Review of attachment 605402 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: allmakefiles.sh > @@ +1,5 @@ > > #! /bin/sh > > # > > +# 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/. > > Please don't do that. > http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/ The license text can be reverted but there have been conflicting requests in this bug to have it changed.
Assignee | ||
Comment 56•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #55) > (In reply to Mike Hommey [:glandium] from comment #54) > > Comment on attachment 605402 [details] [diff] [review] > > cary review forward with req cleanups > > > > Review of attachment 605402 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: allmakefiles.sh > > @@ +1,5 @@ > > > #! /bin/sh > > > # > > > +# 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/. > > > > Please don't do that. > > http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/ > > The license text can be reverted but there have been conflicting requests in > this bug to have it changed. Actually this patch has been lingering for far to long to delay being accepted over comment changes. It has been tested with the MPL replaced and should land in the current form. Will not bother modifying the notice on future edits.
Comment 57•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #55) > (In reply to Mike Hommey [:glandium] from comment #54) > > Comment on attachment 605402 [details] [diff] [review] > > cary review forward with req cleanups > > > > Review of attachment 605402 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: allmakefiles.sh > > @@ +1,5 @@ > > > #! /bin/sh > > > # > > > +# 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/. > > > > Please don't do that. > > http://blog.gerv.net/2012/03/mpl-boilerplate-at-mozilla-faq/ > > The license text can be reverted but there have been conflicting requests in > this bug to have it changed. Gerv is the authority on that. > Actually this patch has been lingering for far to long to delay being > accepted over comment changes. It has been tested with the MPL replaced and > should land in the current form. Will not bother modifying the notice on > future edits. Are you saying you're afraid that changing a comment/license boilerplate is going to break something?
Assignee | ||
Comment 58•12 years ago
|
||
> > Actually this patch has been lingering for far to long to delay being
> > accepted over comment changes. It has been tested with the MPL replaced and
> > should land in the current form. Will not bother modifying the notice on
> > future edits.
>
> Are you saying you're afraid that changing a comment/license boilerplate is
> going to break something?
No a simple comment change *should* not break anything. But modifying a working, tested and reviewed patch, to revert ?-verbatim-? text edit that will eventually be inserted. Seems like busy work.
Since changes have been made, even trivial ones, the patch will need to be re-tested.
So submit to try and see what other random/transient failures are logged, possibly becoming blocked on those along the way.
If the patch edits somehow become blocked on the bulk MPL edits gerv is working on there may also be a delay introduced there. He did mention encountering build & test problems in the try submission that need to be sorted out.
I do not think blocking finished work on pending work is a good precedent to set.
Assignee | ||
Comment 59•12 years ago
|
||
these makefile edits should simmer in b-s for a while.
Comment 60•12 years ago
|
||
Comment on attachment 605402 [details] [diff] [review] cary review forward with req cleanups https://hg.mozilla.org/projects/build-system/rev/9c293bf4df91
Comment 61•12 years ago
|
||
All the failures seem to be due to missing files which just came over in the merge from m-c (thanks philor).
Comment 62•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0fe55a35369f accidentally adds config/rules.mk.orig and js/src/config/rules.mk.orig
Assignee | ||
Comment 63•12 years ago
|
||
Patch landed on mozilla-central yesterday by Coop & Philor: https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Comment 64•12 years ago
|
||
It seems that you accidentally add two .orig files: +++ b/config/rules.mk.orig +++ b/js/src/config/rules.mk.orig This would probably not have happened if my file adding .orig files to .hgignore had been accepted... see bug 634029.
New bug please.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 66•12 years ago
|
||
(In reply to Takanori MATSUURA from comment #62) (In reply to Benoit Jacob [:bjacob] from comment #64) Filed as bug 743433.
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•