Closed Bug 746277 Opened 8 years ago Closed 7 years ago

Replace shell pipelines with make logic around hg invocations

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: joey, Assigned: joey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

toolkit/mozapps/installer/package-name.mk

"MOZ_SOURCE_REPO=" logic can be rewritten using one shell command and make string functions.
Extract identical logic from toolkit/xre/Makefile.in and move into a config/makefiles/ library for sharing.

A parameterized function can probably replace most of these calls:
toolkit/content/Makefile.in
toolkit/xre/Makefile.in
build/Makefile.in
b2g/app/Makefile.in
Assignee: nobody → joey
o Moved hg repo query logic into a make user function.
o Function defined by config/makefiles/rcs.mk {~revision control}
o Common makefile config/makefiles/rules-pre.mk added to conditionally load makefiles like rc.mk on demand.  This makefile differs from rules.mk in that it can be included very early, makefiles loaded should only contain user functions and standalone content.
o Unit test added to verify $(call getSourceRepo) & $(call getSourceRepo,$(path))
o Makefile.in edits verified by inline string tests:
ifneq ($(old),$(new))
  $(error invalid path detected $(old) != $(new))
endif

o Shell pipeline commands removed.  getRepoSource will run a single shell command then massage content using make string functions.
Attachment #616726 - Flags: review?(ted.mielczarek)
Blocks: 747147
ping on the code review
Comment on attachment 616726 [details] [diff] [review]
Reduce shell cmd usage when querying for 'hg showconfig paths.default'

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

r- for a few things but the core concept is sound.

::: b2g/app/Makefile.in
@@ +91,5 @@
>    -DMOZ_UPDATER=$(MOZ_UPDATER) \
>    $(NULL)
>  
> +SOURCE_REPO := $(call getSourceRepo,$(srcdir)/..)
> +# $(call checkIfEmpty,SOURCE_REPO)

Either put this in or don't, but don't leave commented-out code.

::: build/Makefile.in
@@ +102,5 @@
>  DEFINES += -DMOZ_SOURCE_STAMP="$(MOZ_SOURCE_STAMP)"
>  endif
>  
> +SOURCE_REPO :=$(call getSourceRepo,$(topsrcdir)/$(MOZ_BUILD_APP)/..)
> +# $(call checkIfEmpty,SOURCE_REPO)

Same thing here.

::: config/makefiles/makeutils.mk
@@ +44,5 @@
> +# Usage: $(call errorIfEmpty,foo NULL bar)
> +errorIfEmpty =$(call checkIfEmpty,error $(argv))
> +warnIfEmpty  =$(call checkIfEmpty,warning $(argv))
> +
> +endif

These hunks seem to show up in all your patches, and I'm never quite sure what's going on there. Are you making changes there? Is it just an artifact of how you're generating your patches?

::: config/makefiles/rcs.mk
@@ +20,5 @@
> +#   directory paths may be passed as an argument.
> +# Usage: $(call getSourceRepo[,repo_dir|args])
> +# Args:
> +#      $(null): repository path for the tree
> +#   ../foo/bar: cd $(dir) && hg showconfig paths.default

I don't understand your argument description format, to be honest. I would probably write this like:
# Args:
#   path (optional): repository to query. Defaults to $(topsrcdir).

@@ +21,5 @@
> +# Usage: $(call getSourceRepo[,repo_dir|args])
> +# Args:
> +#      $(null): repository path for the tree
> +#   ../foo/bar: cd $(dir) && hg showconfig paths.default
> +getSourceRepo =$(call FUNC_getSourceRepo,$(if $(1),cd $(1) && hg,hg --repository $(topsrcdir)))

Seems a little weird to use "cd dir && hg" vs "hg --repository" based solely on whether an argument was provided. I'm not sure why we weren't using "hg --repository" in the first place, I'd have to go back and look at the original bugs. (It's possible it was to support old versions of hg that don't matter any more.) Either way we should be consistent here and use the same thing in both cases.

@@ +28,5 @@
> +  $(patsubst %/,%,\
> +  $(patsubst ssh://%,http://%,\
> +  $(strip \
> +    $(firstword $(shell $(getargv) showconfig paths.default))\
> +    )))

Translating that sed into patsubst makes this way cleaner. That's a nice improvement!

::: config/makefiles/rules-pre.mk
@@ +24,5 @@
> +include $(topsrcdir)/config/makefiles/makeutils.mk
> +
> +ifndef INCLUDED_RCS_MK
> +  include $(topsrcdir)/config/makefiles/rcs.mk
> +endif

I'm not sure I see the value of this file. Why not just have makeutils.mk include rcs.mk directly? (Or just stick the contents of rcs.mk into makeutils.mk wholesale, possibly ifdef USE_RCS_MK or some such thing.)
Attachment #616726 - Flags: review?(ted.mielczarek) → review-
Summary: fennec: toolkit/mozapps/installer/package-name.mk: reduce shell command usage → Replace shell pipelines with make logic around hg invocations
(In reply to Ted Mielczarek [:ted] from comment #4)
> Comment on attachment 616726 [details] [diff] [review]
> Reduce shell cmd usage when querying for 'hg showconfig paths.default'
> 
> Review of attachment 616726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for a few things but the core concept is sound.

Thanks
 
> ::: b2g/app/Makefile.in
> @@ +91,5 @@
> >    -DMOZ_UPDATER=$(MOZ_UPDATER) \
> >    $(NULL)
> >  
> > +SOURCE_REPO := $(call getSourceRepo,$(srcdir)/..)
> > +# $(call checkIfEmpty,SOURCE_REPO)
> 
> Either put this in or don't, but don't leave commented-out code.

The check should be enabled/is a todo item, but I've not done enough testing yet to verify that exit-on-empty will not cause fallout.  At a minimum a b2g build will be needed.


> ::: config/makefiles/makeutils.mk
> @@ +44,5 @@
> > +# Usage: $(call errorIfEmpty,foo NULL bar)
> > +errorIfEmpty =$(call checkIfEmpty,error $(argv))
> > +warnIfEmpty  =$(call checkIfEmpty,warning $(argv))
> > +
> > +endif
> 
> These hunks seem to show up in all your patches, and I'm never quite sure
> what's going on there. Are you making changes there? Is it just an artifact
> of how you're generating your patches?

The *IfEmpty() functions were part of a makeutils.mk library patch that, at the time of this upload, was either blocked on review or landing I forget wich { some funkyness went on with the original mega-makefile merge/patch that landed in b-s and later m-c -- one merge added the the functions and a subsequent op removed them ?!? }.  Bug # 746151 is a chaser job to re-add the missing pieces ontop of mozilla-central.

Rather than block several other patches dependent on the makeutils::*IfEmpty library functions I just inlined a copy of them for now to remove the blocking factor.


> ::: config/makefiles/rcs.mk
> @@ +20,5 @@
> > +#   directory paths may be passed as an argument.
> > +# Usage: $(call getSourceRepo[,repo_dir|args])
> > +# Args:
> > +#      $(null): repository path for the tree
> > +#   ../foo/bar: cd $(dir) && hg showconfig paths.default
> 
> I don't understand your argument description format, to be honest. I would
> probably write this like:
> # Args:
> #   path (optional): repository to query. Defaults to $(topsrcdir).

Ok good edit for documentation clarity.


> @@ +21,5 @@
> > +# Usage: $(call getSourceRepo[,repo_dir|args])
> > +# Args:
> > +#      $(null): repository path for the tree
> > +#   ../foo/bar: cd $(dir) && hg showconfig paths.default
> > +getSourceRepo =$(call FUNC_getSourceRepo,$(if $(1),cd $(1) && hg,hg --repository $(topsrcdir)))
> 
> Seems a little weird to use "cd dir && hg" vs "hg --repository" based solely
> on whether an argument was provided. I'm not sure why we weren't using "hg
> --repository" in the first place, I'd have to go back and look at the
> original bugs. (It's possible it was to support old versions of hg that
> don't matter any more.) Either way we should be consistent here and use the
> same thing in both cases.

This syntax was purely to emulate existing syntax used in different makefiles.  If I remember correctly use of --repository from one of the subdirs -vs- cd $blah && hg cmds -- returned an odd path with "patches/" and/or ${username}/ appended.

If we can lock down the functionality initially with testing.  Update tests to cover all paths and verify they return expected values it should then be safe in a followup patch to make usage consistent.


> @@ +28,5 @@
> > +  $(patsubst %/,%,\
> > +  $(patsubst ssh://%,http://%,\
> > +  $(strip \
> > +    $(firstword $(shell $(getargv) showconfig paths.default))\
> > +    )))
> 
> Translating that sed into patsubst makes this way cleaner. That's a nice
> improvement!

heh which do you like better 4 shells or 1 :)


> ::: config/makefiles/rules-pre.mk
> @@ +24,5 @@
> > +include $(topsrcdir)/config/makefiles/makeutils.mk
> > +
> > +ifndef INCLUDED_RCS_MK
> > +  include $(topsrcdir)/config/makefiles/rcs.mk
> > +endif
> 
> I'm not sure I see the value of this file. Why not just have makeutils.mk
> include rcs.mk directly? (Or just stick the contents of rcs.mk into
> makeutils.mk wholesale, possibly ifdef USE_RCS_MK or some such thing.)

I'm using the conditionals to setup a bit of environment isolation within the makefiles.  Rather than pull in everything but the kitchen sink { with potential interactions } a makefile can indicate when it needs access to the extra special functionality.

Only a handful of makefiles currently need access to the revision control commands.  If they define INCLUDED_RCS_MK=1 it will allow all of the other tree makefiles to be blissfully unaware of the revision control defs.
(In reply to Joey Armstrong [:joey] from comment #5)
> The check should be enabled/is a todo item, but I've not done enough testing
> yet to verify that exit-on-empty will not cause fallout.  At a minimum a b2g
> build will be needed.

Okay, then perhaps put a TODO comment in there instead? I'm not a fan of leaving in commented-out code, whatever the reason behind it.

> Rather than block several other patches dependent on the makeutils::*IfEmpty
> library functions I just inlined a copy of them for now to remove the
> blocking factor.

Ok, that's fine. We should just get those landed if they keep being a problem for you.

> This syntax was purely to emulate existing syntax used in different
> makefiles.  If I remember correctly use of --repository from one of the
> subdirs -vs- cd $blah && hg cmds -- returned an odd path with "patches/"
> and/or ${username}/ appended.
> 
> If we can lock down the functionality initially with testing.  Update tests
> to cover all paths and verify they return expected values it should then be
> safe in a followup patch to make usage consistent.

The only place that uses --repository is the one you previously changed:
http://hg.mozilla.org/mozilla-central/rev/106c7696d3b7

That doesn't seem to have caused any problems, so maybe just switch to that everywhere?

> I'm using the conditionals to setup a bit of environment isolation within
> the makefiles.  Rather than pull in everything but the kitchen sink { with
> potential interactions } a makefile can indicate when it needs access to the
> extra special functionality.

That seems fine, but it can still live inside makeutils.mk, or makeutils.mk can include rcs.mk directly. I'm okay with some indirection, but adding two new makefiles just for this one function seems excessive.
(In reply to Ted Mielczarek [:ted] from comment #6)
> (In reply to Joey Armstrong [:joey] from comment #5)
> > The check should be enabled/is a todo item, but I've not done enough testing
> > yet to verify that exit-on-empty will not cause fallout.  At a minimum a b2g
> > build will be needed.
> 
> Okay, then perhaps put a TODO comment in there instead? I'm not a fan of
> leaving in commented-out code, whatever the reason behind it.

todo


> > I'm using the conditionals to setup a bit of environment isolation within
> > the makefiles.  Rather than pull in everything but the kitchen sink { with
> > potential interactions } a makefile can indicate when it needs access to the
> > extra special functionality.
> 
> That seems fine, but it can still live inside makeutils.mk, or makeutils.mk
> can include rcs.mk directly. I'm okay with some indirection, but adding two
> new makefiles just for this one function seems excessive.

There is also a bit of unseen hierarchy in play with these two makefiles.  makeutils.mk has been included by most of the new makefile unit tests so it has been maintained as a set of standalone/dependency-free functions.

More task/element specific makefiles will likely need to be added later on to refactor them into a hierarchy.  If makeutils.mk were to act as the common loader for all the includes it will increase the chance for bad test interactions.
I understand your motivation, I'd just like to avoid adding lots of extra Makefiles for little gain. Adding two separate makefiles just for this one function seems like overkill right now.
(In reply to Ted Mielczarek [:ted] from comment #6)
> (In reply to Joey Armstrong [:joey] from comment #5)
> > This syntax was purely to emulate existing syntax used in different
> > makefiles.  If I remember correctly use of --repository from one of the
> > subdirs -vs- cd $blah && hg cmds -- returned an odd path with "patches/"
> > and/or ${username}/ appended.
> > 
> The only place that uses --repository is the one you previously changed:
> http://hg.mozilla.org/mozilla-central/rev/106c7696d3b7
> 
> That doesn't seem to have caused any problems, so maybe just switch to that
> everywhere?

How about opening another bug and consolidating argument usage in a followup patch ?  (cd && hg -vs- hg --repository) are returning distinct URLs for the same directory.  hg --repository returning the URL for a local patch directory could become a bug source.

[phantasm:tests] pwd
/local/mozilla/bugs/746277/dbm/tests

[phantasm:tests] hg --repository `pwd`  show paths.default
http://hg.mozilla.org/users/${login}_mozilla.com/patches

[phantasm:tests] hg show paths.default
http://hg.mozilla.org/mozilla-central

[phantasm:tests] hg --repository ../tests/.  show paths.default
http://hg.mozilla.org/users/${login}_mozilla.com/patches
All of the callers here are either passing a directory or getting $(topsrcdir), so I don't expect that to be a problem.
Also, as I noted before, the only instance of using --repository is code you've already touched, so what difference does it make?
(In reply to Ted Mielczarek [:ted] from comment #11)
> Also, as I noted before, the only instance of using --repository is code
> you've already touched, so what difference does it make?

Depending on the path argument passed { $(DEPTH), $(topsrcdir), etc } hg --repository can return different paths.  If during a release the argument flavor aware of a users local patch directory were used (hg --repository), and patches have been applied, a non-standard path would be returned.

http://hg.mozilla.org/users/${login}_mozilla.com/patches

Brand this path into platform.ini and people may no longer be able to access the URL { or anything derived from it }.
That's an exceedingly unlikely scenario, FWIW, but fine, I withdraw my objection.
Attachment #616726 - Attachment is obsolete: true
Comment on attachment 624485 [details] [diff] [review]
replace hg shell pipelines with make logic

re-merge with m-c
re-tested on try: https://tbpl.mozilla.org/?tree=Try&rev=db8e3f0e3755
Failures reported are from: tests/cpp/test_IHistory.cpp
Edits should be minor from the last patch reviewed.

{check,error,warn}IfEmpty() hunks are no longer a problem, bug adding them has landed.  Repo var checking logic added as $(warnIfEmpty ) in case tb/seamonkey does not require the dependency or people are using an older version of hg locally.

removed rules-pre.mk and added a conditional include in makeutils.mk.
unit test setup for getSourceRepo() function and deps setup to only run the unit test when rcs.mk or a dependent makefile is modified.
Attachment #624485 - Flags: review?(ted.mielczarek)
ping on code review
ping on code review from the 16th
Comment on attachment 624485 [details] [diff] [review]
replace hg shell pipelines with make logic

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

Sorry for the long delay on this, I've been pretty bad about reviews in general the past few weeks. Just a few minor nits.

::: b2g/app/Makefile.in
@@ +89,5 @@
>    -DAPP_VERSION=$(MOZ_APP_VERSION) \
>    -DMOZ_UPDATER=$(MOZ_UPDATER) \
>    $(NULL)
>  
> +SOURCE_REPO := $(call getSourceRepo,$(srcdir)/..)

Did you want to make these variables lowercase while you're at it, since they're file-local? Not really a big deal.

::: build/Makefile.in
@@ +100,5 @@
>  ifdef MOZ_SOURCE_STAMP
>  DEFINES += -DMOZ_SOURCE_STAMP="$(MOZ_SOURCE_STAMP)"
>  endif
>  
> +SOURCE_REPO :=$(call getSourceRepo,$(topsrcdir)/$(MOZ_BUILD_APP)/..)

uber-nit: You're inconsistent about your spacing after the := in various Makefiles here.

::: config/makefiles/test/check-rcs.mk
@@ +26,5 @@
> +	exit $$rc
> +	@rc=0; \
> +	if [ ! "$(call getSourceRepo,../..)" ]; then \
> +	    echo "ERROR: rcs.mk::getSourceRepo(../..) failed"; \
> +	    rc=1; \

Are these going to error if someone runs "make check" in a source directory that's not a hg repo? That would be bad.
Attachment #624485 - Flags: review?(ted.mielczarek) → review+
Attachment #624485 - Attachment is obsolete: true
Comment on attachment 633149 [details] [diff] [review]
replace hg shell pipelines with make logic

Patch very similar to last approved version but addressing one important nit that was in the review comments.

o check-rcs.mk unit test updated to no longer fail in a non-hg sandbox.
o Localized SOURCE_REPO vars as source_repo.
o Added function getRcsType().  Initial version can detect git and mercurial.
  Logic used to conditionally run check-rcs.mk unit test.
  Logic used to conditionally define commands ~getSourceRepo() based on type of local revision control.
o Removed warnIfEmpty(source_repo) checks, no longer needed since git can return 'source repo' value.
  - Most makefiles will not use source_repo in a git sandbox because of the 'git:' prefix, build/Makefile.in is the exception.  Changing if 'http' conditionals to "if != $(NULL)" would help remove special cases.
o Reversed order of INCLUDED_RCS_MK and USE_RCS_MK ifdefs conditionals in config/makefiles/rcs.mk to prevent a potential lockout condition.
o converted check-rcs.mk unit test into a compile time test for easier control over when the test should run.  target+shell_cmds would require playing games with targets and preqs.
o Optimized conditional used to detect a remote source repository URL.  if $(filter http:%) -vs- ifeq (http,$(patsubst http%,http))

A try job is in the pipeline.
Attachment #633149 - Flags: review?(ted.mielczarek)
Comment on attachment 633149 [details] [diff] [review]
replace hg shell pipelines with make logic

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

::: config/makefiles/rcs.mk
@@ +55,5 @@
> +###########################################################################
> +ifneq (,$(findstring .git,$(MOZ_RCS_TYPE)))
> +  # git://github.com/mozilla/...
> +  getSourceRepo = $(firstword $(shell $(getargv) git config --get remote.origin.url))
> +endif #} HAVE_GIT_RCS

I don't think this buys us much, honestly. None of the places calling this are really equipped to deal with git repos at the moment. This is probably fine for now, since as you noted all the callers are filtering for HTTP anyway, but we'd need a bit more work to actually support git using this. (And it's not worthwhile right now, since we don't have any release builds being built from a git repo.)
Attachment #633149 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted] from comment #22)
> Comment on attachment 633149 [details] [diff] [review]
> replace hg shell pipelines with make logic
> 
> Review of attachment 633149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/makefiles/rcs.mk
> @@ +55,5 @@
> > +###########################################################################
> > +ifneq (,$(findstring .git,$(MOZ_RCS_TYPE)))
> > +  # git://github.com/mozilla/...
> > +  getSourceRepo = $(firstword $(shell $(getargv) git config --get remote.origin.url))
> > +endif #} HAVE_GIT_RCS
> 
> I don't think this buys us much, honestly. None of the places calling this
> are really equipped to deal with git repos at the moment. This is probably
> fine for now, since as you noted all the callers are filtering for HTTP
> anyway, but we'd need a bit more work to actually support git using this.
> (And it's not worthwhile right now, since we don't have any release builds
> being built from a git repo.)

Longer term I think there are benefits to be had from these conditional command definitions.  At a minimum we now have a framework in place for either sandbox type to support functionality and unit testing.  Going forward { more importantly } we can dismantle/replace the mercurial-only command blocks that contribute to variant builds based on revision control type.

thanks for the review
Attachment #633149 - Attachment is obsolete: true
r=ted carried forward.
Minor edit, explicitly assign MOZ_RCS_TYPE when calling getRcsType

Try job: https://tbpl.mozilla.org/?tree=Try&rev=8bb391810c54
inbound push: 6a386d638a1a: bug 746277: replace hg shell pipelines with make logic default tip
Blocks: 769390
(In response to email)

> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/06e7df3a8209

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6a386d638a1a

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13040121&tree=Mozilla-Inbound
{
========= Started no change (results: 2, elapsed: 0 secs) (at 2012-06-27 08:13:40.644016) =========
python /Users/cltbld/talos-slave/test/tools/buildfarm/utils/printbuildrev.py ./FirefoxNightlyDebug.app/Contents/MacOS
 in dir /Users/cltbld/talos-slave/test/build (timeout 1200 secs)
 watching logfiles {}
 argv: ['python', '/Users/cltbld/talos-slave/test/tools/buildfarm/utils/printbuildrev.py', './FirefoxNightlyDebug.app/Contents/MacOS']
 environment:
  Apple_PubSub_Socket_Render=/tmp/launch-JJczdw/Render
  CVS_RSH=ssh
  DISPLAY=/tmp/launch-qxgF0Y/org.x:0
  HOME=/Users/cltbld
  LOGNAME=cltbld
  PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin
  PWD=/Users/cltbld/talos-slave/test/build
  PYTHONPATH=/Library/Python/2.5/site-packages
  SHELL=/bin/bash
  SSH_AUTH_SOCK=/tmp/launch-3ncWVG/Listeners
  TMPDIR=/var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/
  USER=cltbld
  VERSIONER_PYTHON_PREFER_32_BIT=no
  VERSIONER_PYTHON_VERSION=2.6
  __CF_USER_TEXT_ENCODING=0x1F5:0:0
 using PTY: False
Traceback (most recent call last):
  File "/Users/cltbld/talos-slave/test/tools/buildfarm/utils/printbuildrev.py", line 19, in <module>
    app_repo = appini.get('App', 'SourceRepository')
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/ConfigParser.py", line 540, in get
    raise NoOptionError(option, section)
ConfigParser.NoOptionError: No option 'sourcerepository' in section: 'App'
program finished with exit code 1
elapsedTime=0.141413

========= Finished no change (results: 2, elapsed: 0 secs) (at 2012-06-27 08:13:40.922224) =========
}
Attachment #634986 - Attachment is obsolete: true
Attachment #667004 - Attachment is obsolete: true
Comment on attachment 667011 [details] [diff] [review]
replace hg commands with functions

removed makefile unit test.

Removed .git logic and changed detection to lookup rcs type once then cache.  Answer retrieved can change based on opendir error or order(.git|.hg).

Exhaustive testing to verify identical repo strings are returned.

Added conditional in makeutils.mk to include rcs logic when USE_RCS_MK is defined.

nti cleanup from the code review script, whitespace around line continuation, etc.
rules.mk: removed redundant autotargets.mk include.
Attachment #667011 - Flags: review?(ted.mielczarek)
https://tbpl.mozilla.org/?tree=Try&rev=da78d49faaa3

pased on linux64-debug.  linux64-opt failed with a timeout in mock_mozilla and an uncaught js exception.
ping on the code review
ping on the code review
Sorry, a little behind on reviews at the moment. Hoping to get them cleared out today.
Do you have an eta for the review ?
Comment on attachment 667011 [details] [diff] [review]
replace hg commands with functions

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

Sorry for the delay. Patch looks good, just a few nits.

::: b2g/app/Makefile.in
@@ +7,5 @@
>  srcdir    = @srcdir@
>  VPATH     = @srcdir@
>  
>  include $(DEPTH)/config/autoconf.mk
> +USE_RCS_MK=1

nit: could use :=1 for these assignments.

::: config/makefiles/rcs.mk
@@ +24,5 @@
> +#   path (optional): repository to query.  Defaults to $(topsrcdir)
> +getSourceRepo = \
> +  $(call FUNC_getSourceRepo,$(if $(1),cd $(1) && hg,hg --repository $(topsrcdir)))
> +
> +# http://hg.mozilla.org/mozilla-central

What's this URL here for?

::: toolkit/mozapps/installer/package-name.mk
@@ +145,5 @@
> +# += $(SPACE): preserve existing functionality.
> +# When MOZILLA_DIR is null and ~/.hg exists an unknown path can be obtained
> +# via cd && hg.  getSourceRepo() defaults to topsrcdir when a value is not
> +# passed so suffix $(SPACE) to obtain the current answer.
> +# TODO: mimic current functionality, migrate to hg --repo once stable.

If you're going to leave a TODO comment I generally prefer if you file a bug on the work and mention the bug number in the comment.

::: toolkit/xre/Makefile.in
@@ +212,5 @@
>  DEFINES += -DHAVE_USR_LIB64_DIR
>  endif
>  endif
>  
> +

nit: extraneous blank line

@@ +222,2 @@
>    # command set should change based on revision control use.
>    # warn for now in case (git, bzr, ...) is in use.

This comment doesn't make sense anymore.
Attachment #667011 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted] from comment #37)
> Comment on attachment 667011 [details] [diff] [review]
> replace hg commands with functions
> 
> Review of attachment 667011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. Patch looks good, just a few nits.
> 
> ::: b2g/app/Makefile.in
> @@ +7,5 @@
> >  srcdir    = @srcdir@
> >  VPATH     = @srcdir@
> >  
> >  include $(DEPTH)/config/autoconf.mk
> > +USE_RCS_MK=1
> 
> nit: could use :=1 for these assignments.
> 
> ::: config/makefiles/rcs.mk
> @@ +24,5 @@
> > +#   path (optional): repository to query.  Defaults to $(topsrcdir)
> > +getSourceRepo = \
> > +  $(call FUNC_getSourceRepo,$(if $(1),cd $(1) && hg,hg --repository $(topsrcdir)))
> > +
> > +# http://hg.mozilla.org/mozilla-central
> 
> What's this URL here for?

Just sample data the call may return.

 
> ::: toolkit/mozapps/installer/package-name.mk
> @@ +145,5 @@
> > +# += $(SPACE): preserve existing functionality.
> > +# When MOZILLA_DIR is null and ~/.hg exists an unknown path can be obtained
> > +# via cd && hg.  getSourceRepo() defaults to topsrcdir when a value is not
> > +# passed so suffix $(SPACE) to obtain the current answer.
> > +# TODO: mimic current functionality, migrate to hg --repo once stable.
> 
> If you're going to leave a TODO comment I generally prefer if you file a bug
> on the work and mention the bug number in the comment.

I'll open one shortly and prune down the comments.

> ::: toolkit/xre/Makefile.in
> @@ +212,5 @@
> >  DEFINES += -DHAVE_USR_LIB64_DIR
> >  endif
> >  endif
> >  
> > +
> 
> nit: extraneous blank line
> 
> @@ +222,2 @@
> >    # command set should change based on revision control use.
> >    # warn for now in case (git, bzr, ...) is in use.
> 
> This comment doesn't make sense anymore.

nits to be cleaned up, thanks.
Attachment #667011 - Attachment is obsolete: true
Comment on attachment 671900 [details] [diff] [review]
replace hg commands with functions

nit fixes.
moved todo text into a bug.
r=ted carried forward.
Pushed to build-system:

Check-in: http://hg.mozilla.org/projects/build-system/rev/a2b82cb13034 - Joey Armstrong - bug 746277: replace hg commands with functions
See Also: → 802188
Attachment #671900 - Attachment is obsolete: true
Comment on attachment 671900 [details] [diff] [review]
replace hg commands with functions

Patch refresh, migrating from build-system to inbound.
Cookbook:
  o add config/makefiles/rcs.mk to hold revision control logic for conditional include.
  o move source_repo=`hg showconfig | sed` calls into a named function.
  o "cd $(dir) && hg" -vs- "hg --repo $(dir)" - conditional support for bug 802188.

r=ted carried forward.
sanity check with a feedback request.
Attachment #671900 - Flags: feedback?(mshal)
Try results with last refresh:
https://tbpl.mozilla.org/?tree=Try&rev=ffb0b30ee88a

A handful of orange/intermittent failures + fedora debug build leaking a few bytes.
inbound: committed changeset 122069:f4cffc9cafba
https://hg.mozilla.org/mozilla-central/rev/f4cffc9cafba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(In reply to Joey Armstrong [:joey] from comment #43)
> Patch refresh, migrating from build-system to inbound.
> Cookbook:
>   o "cd $(dir) && hg" -vs- "hg --repo $(dir)" - conditional support for bug
> 802188.

This whole patch broke comm-central badly, even with that support added in:

   o build/Makefile.in includes makeutils.mk from topsrcdir instead of MOZILLA_DIR (but at a point where MOZILLA_DIR is not defined)
   o package-name.mk is including mkutils.mk directly in comm-* directories, where topsrcdir is definitely comm* instead of mozilla*.

Since this bug is not critical, and MERGE DAY is tuesday - I suggest backout until these issues can be sorted out, since fixing them is not trivial from the comm-* side alone.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(joey)
(In reply to Justin Wood (:Callek) from comment #47)
> Since this bug is not critical, and MERGE DAY is tuesday - I suggest backout
> until these issues can be sorted out, since fixing them is not trivial from
> the comm-* side alone.

Bug 842106 did a hack solution for comm-*, basically copying mkutils.mk and rcs.mk to comm-central, it is a far larger hack than I'd have liked though because this means that m-c makefiles will be calling the c-c mkutils and rcs in multiple places.
(In reply to Justin Wood (:Callek) from comment #48)
> Bug 842106 did a hack solution for comm-*

Both Thunderbird and SeaMonkey trunk tinderboxes remain broken though despite that mitigating fix, thus the issue hasn't been not solved just by that alone.
(In reply to rsx11m from comment #49)
> (In reply to Justin Wood (:Callek) from comment #48)
> > Bug 842106 did a hack solution for comm-*
> 
> Both Thunderbird and SeaMonkey trunk tinderboxes remain broken though
> despite that mitigating fix, thus the issue hasn't been not solved just by
> that alone.

...separate bug.
Oops, sorry then.
(In reply to Justin Wood (:Callek) from comment #48)
> (In reply to Justin Wood (:Callek) from comment #47)
> > Since this bug is not critical, and MERGE DAY is tuesday - I suggest backout
> > until these issues can be sorted out, since fixing them is not trivial from
> > the comm-* side alone.
> 
> Bug 842106 did a hack solution for comm-*, basically copying mkutils.mk and
> rcs.mk to comm-central, it is a far larger hack than I'd have liked though
> because this means that m-c makefiles will be calling the c-c mkutils and
> rcs in multiple places.

Question: What would be the problem with copying a set of files into the comm-central sandbox ?  I would think this could provide some versioning insulation so comm-central would not need to react so quickly when invasive changes are introduced.

If remaining current with m-c is the underlying problem how about simply replacing config/makefiles with a symlink to the appropriate directory beneath MOZILLA_BUILD.  If makefiles are accessed in exactly the same way between all sandboxes it should help remove conditionals/special case code so behavior will be consistent and easier to maintain.
Flags: needinfo?(joey)
(In reply to Joey Armstrong [:joey] from comment #52)
> (In reply to Justin Wood (:Callek) from comment #48)
> > (In reply to Justin Wood (:Callek) from comment #47)
> > > Since this bug is not critical, and MERGE DAY is tuesday - I suggest backout
> > > until these issues can be sorted out, since fixing them is not trivial from
> > > the comm-* side alone.
> > 
> > Bug 842106 did a hack solution for comm-*, basically copying mkutils.mk and
> > rcs.mk to comm-central, it is a far larger hack than I'd have liked though
> > because this means that m-c makefiles will be calling the c-c mkutils and
> > rcs in multiple places.
> 
> Question: What would be the problem with copying a set of files into the
> comm-central sandbox ?

That we need to do the mozilla/ directory stuff in many cases, and that porting changes adds up human time fast! That said, Standard8 did said port and (as I didn't notice) since these are in their own subdirectory we can do the "ensure stuff matches" code, also because none of these files (yet) needs us to add mozilla/

> I would think this could provide some versioning
> insulation so comm-central would not need to react so quickly when invasive
> changes are introduced.

Sadly, atm, we don't have that luxury, since much of comm-central needs to get linked into libxul, we build libxul modules from comm-central, so we need matching stuff, but we also need to maintain the ability for the vice-versa (stuff in m-c being built from comm, e.g. extensions, etc.) Its an ugly mess right now.

> If remaining current with m-c is the underlying problem how about simply
> replacing config/makefiles with a symlink to the appropriate directory
> beneath MOZILLA_BUILD. 

Symlinks don't work on windows

> If makefiles are accessed in exactly the same way
> between all sandboxes it should help remove conditionals/special case code
> so behavior will be consistent and easier to maintain.

In theory yes, but sadly the theory doesn't stack to reality right now.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Attachment #671900 - Flags: feedback?(mshal)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.