Closed
Bug 738404
Opened 12 years ago
Closed 12 years ago
Makefile.in update to use 680246 - file batch #1
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, 1 obsolete file)
29.15 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
Target rule mkdir calls replaced with $(call mkdir_deps) dependencies [~bug 680246]. Cosmetic cleanups: replaced rm -f with $(RM) in a few makefiles. config/config.mk - fixed a quoting problem affecting editor colorization.
Attachment #608452 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 2•12 years ago
|
||
Try job + patches: 680246, 7634121 & 735638 =========================================================== Thanks for your try submission (http://hg.mozilla.org/try/pushloghtml?changeset=22a3e5ba61cd). It's the best! Watch https://tbpl.mozilla.org/?tree=Try&rev=22a3e5ba61cd 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-22a3e5ba61cd. This directory won't be created until the first builds are uploaded, so please be patient.
Comment 3•12 years ago
|
||
Comment on attachment 608452 [details] [diff] [review] Makefile edits to begin using threadsafe mkdir logic from bug 680246 Review of attachment 608452 [details] [diff] [review]: ----------------------------------------------------------------- Overall I'm fine with this patch, I'm just r-ing to get answers to the first two questions here. ::: b2g/app/Makefile.in @@ +130,5 @@ > +.PHONY: repackage > +libs repackage:: \ > + $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/MacOS) \ > + $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/Resources/$(AB).lproj) \ > + $(NULL) The formatting on these dependencies come off a little confusing. Is there a way to do this that doesn't require the line continuations after the ::? Maybe use an intermediate variable and list it as the deps? ::: build/win32/Makefile.in @@ +102,5 @@ > endif > > ifdef REDIST_FILES > +libs:: \ > + $(call mkdir_deps,$(FINAL_TARGET)) \ Is there a reason you're using these $(call mkdir_deps) directly instead of just using GENERATED_DIRS and GENERATED_DIR_DEPS? ::: config/Makefile.in @@ +192,5 @@ > unit-writemozinfo.py \ > $(NULL) > > +check:: \ > + check-python-modules check-jar-mn \ Might as well reformat this to one-dependent-target-per-line while you're here. ::: config/config.mk @@ +45,5 @@ > # > > # Define an include-at-most-once flag > ifdef INCLUDED_CONFIG_MK > +$(error Do not include config.mk twice!) I'm not opposed to lumping cleanup in a patch, but it does make it easier to review if you localize your cleanup to the places you're already touching, FWIW. ::: config/rules.mk @@ +480,5 @@ > # A Makefile that needs $(MDDEPDIR) created but doesn't set any of these > # variables we know to check can just set NEED_MDDEPDIR explicitly. > ifneq (,$(OBJS)$(XPIDLSRCS)$(SIMPLE_PROGRAMS)$(NEED_MDDEPDIR)) > MAKE_DIRS += $(CURDIR)/$(MDDEPDIR) > +GARBAGE_DIRS += $(MDDEPDIR) This doesn't seem to be a useful change.
Attachment #608452 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Ted Mielczarek [:ted] from comment #3) > Comment on attachment 608452 [details] [diff] [review] > Makefile edits to begin using threadsafe mkdir logic from bug 680246 > > Review of attachment 608452 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall I'm fine with this patch, I'm just r-ing to get answers to the first > two questions here. > > ::: b2g/app/Makefile.in > @@ +130,5 @@ > > +.PHONY: repackage > > +libs repackage:: \ > > + $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/MacOS) \ > > + $(call mkdir_deps,$(DIST)/$(APP_NAME).app/Contents/Resources/$(AB).lproj) \ > > + $(NULL) > > The formatting on these dependencies come off a little confusing. Is there a > way to do this that doesn't require the line continuations after the ::? > Maybe use an intermediate variable and list it as the deps? Formatting was intended to fit 80cpl and avoid line wrap (the coding style guide also mentioned something about it). target: offset 0, pre-reqs: indent 2 spaces, cmds: tab indent Yes vars could be used to shorten the target line. The reason I opted to not use them here was because of scope. Var use would morph the value from local use to global which could become a problem source. If the macro is overwritten or conflicts with one of the included makefiles. > ::: build/win32/Makefile.in > @@ +102,5 @@ > > endif > > > > ifdef REDIST_FILES > > +libs:: \ > > + $(call mkdir_deps,$(FINAL_TARGET)) \ > > Is there a reason you're using these $(call mkdir_deps) directly instead of > just using GENERATED_DIRS and GENERATED_DIR_DEPS? Yes, this syntax is intended to limit dep checking/generation to only targets that depend on it. If several targets require the dep ( export, libs, tools, check, ...) assigning to GENERATED_DIRS would make perfect sense. If 'install' were the only target requiring a dep it would not make sense for 'make check', 'make export', ... to also generate the dep. > > ::: config/Makefile.in > @@ +192,5 @@ > > unit-writemozinfo.py \ > > $(NULL) > > > > +check:: \ > > + check-python-modules check-jar-mn \ > > Might as well reformat this to one-dependent-target-per-line while you're > here. Ok with the next patch upload > ::: config/config.mk > @@ +45,5 @@ > > # > > > > # Define an include-at-most-once flag > > ifdef INCLUDED_CONFIG_MK > > +$(error Do not include config.mk twice!) > > I'm not opposed to lumping cleanup in a patch, but it does make it easier to > review if you localize your cleanup to the places you're already touching, > FWIW. It would be easy enough to pull this edit out of the patch. config/config.mk will be submitted with other edits and could be submitted with an appropriate bug. > ::: config/rules.mk > @@ +480,5 @@ > > # A Makefile that needs $(MDDEPDIR) created but doesn't set any of these > > # variables we know to check can just set NEED_MDDEPDIR explicitly. > > ifneq (,$(OBJS)$(XPIDLSRCS)$(SIMPLE_PROGRAMS)$(NEED_MDDEPDIR)) > > MAKE_DIRS += $(CURDIR)/$(MDDEPDIR) > > +GARBAGE_DIRS += $(MDDEPDIR) > > This doesn't seem to be a useful change. Ugh yes, wip
Comment 5•12 years ago
|
||
(In reply to Joey Armstrong [:joey] from comment #4) > Formatting was intended to fit 80cpl and avoid line wrap (the coding style > guide also mentioned something about it). > target: offset 0, pre-reqs: indent 2 spaces, cmds: tab indent Right, wrapping at 80 columns is fine, I'm just really not a fan of having a line continuation in the prerequisite portion of a rule with a recipe. It makes it really confusing to read. > Yes vars could be used to shorten the target line. The reason I opted to > not use them here was because of scope. Var use would morph the value from > local use to global which could become a problem source. If the macro is > overwritten or conflicts with one of the included makefiles. Ah, yes. This is a fair concern, but I think I would prefer this to avoid the ugly formatting. Perhaps we could come up with variable naming conventions that help us avoid conflicts? Someone suggested perhaps using lowercase variable names for non-global variables. > Yes, this syntax is intended to limit dep checking/generation to only > targets that depend on it. If several targets require the dep ( export, > libs, tools, check, ...) assigning to GENERATED_DIRS would make perfect > sense. If 'install' were the only target requiring a dep it would not make > sense for 'make check', 'make export', ... to also generate the dep. Okay. It's a bit of a pain, but specifying our dependencies more clearly is a sane goal. I didn't have any other issues with this patch, so if you can tweak that one issue it should be an easy r+.
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Ted Mielczarek [:ted] from comment #5) > Right, wrapping at 80 columns is fine, I'm just really not a fan of having a line continuation in the prerequisite portion of a rule with a recipe. It makes it really confusing to read. > > Ah, yes. This is a fair concern, but I think I would prefer this to avoid > the ugly formatting. Perhaps we could come up with variable naming > conventions that help us avoid conflicts? Someone suggested perhaps using lowercase variable names for non-global variables. Lowercase var names for local vars would be a reasonable suggestion. How about this for a thought/convention (lines>80cpl): o Declare a local macro above the rule suffixed by -preqs o Accumulate deps, one per line appended with += o Target rule(s) could then be written as: export-preqs += foo export-preqs += $(call mkdir_deps,bar tans fans) export: $(export-preqs) > I didn't have any other issues with this patch, so if you can tweak that one issue it should be an easy r+. Ok thanks
Comment 7•12 years ago
|
||
Your suggestion sounds totally reasonable. An alternate that would match what we do in a lot of Makefiles would be to use continuations in the variable assignment, like: export-preqs = \ foo $(call mkdir_deps,bar tans fans) \ $(NULL) I don't have a problem with continuations in general, just in the prereqs section of a rule with a recipe. Either way is fine, you can choose.
Assignee | ||
Comment 8•12 years ago
|
||
Target line continuation for pre-requisites moved to named macros {export,libs}-preqs. Common path macros defined in a few places to shorten command paths GARBAGE_DIRS += $(MDDEPDIR) was visible due to a whitespace change, updated removal path.
Attachment #608452 -
Attachment is obsolete: true
Attachment #610175 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 9•12 years ago
|
||
Try job: https://tbpl.mozilla.org/?tree=Try&rev=3b1e4f9bd98d b2g latex2man failure is known, will be fixed by "dont_build_docs.sh"
Updated•12 years ago
|
Attachment #610175 -
Attachment is patch: true
Comment 10•12 years ago
|
||
Comment on attachment 610175 [details] [diff] [review] Makefile.in edits to use bug 680246 functionality Review of attachment 610175 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thanks!
Attachment #610175 -
Flags: review?(ted.mielczarek) → review+
Comment 11•12 years ago
|
||
Comment on attachment 610175 [details] [diff] [review] Makefile.in edits to use bug 680246 functionality https://hg.mozilla.org/projects/build-system/rev/1f093ab9df77
Assignee | ||
Comment 12•12 years ago
|
||
Patch landed on mozilla-central yesterday by Coop & Philor: https://hg.mozilla.org/mozilla-central/rev/c598b7b202e7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 744750
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
•