teach comm-central client.mk to invoke client.py

RESOLVED FIXED in Thunderbird 9.0

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: gozer, Assigned: gozer)

Tracking

unspecified
Thunderbird 9.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

55.83 KB, patch
standard8
: review+
Details | Diff | Splinter Review
1.39 KB, patch
Callek
: review+
standard8
: review+
Details | Diff | Splinter Review
363 bytes, patch
Callek
: review-
Details | Diff | Splinter Review
This patch teaches comm-central's client.mk how to look for CLIENT_PY_ARGS in .mozconfig and invokes client.py automatically if it's there.

i.e.
 mk_add_options CLIENT_PY_ARGS="-v checkout --skip-venkman"

The reason for landing something like this is so that comm-central can become the same as mozilla-central builds from buildbot's POV.

Nothing special about these builds, except for a different .mozconfig. This way, buildbot itself won't need to know to invoke client.py, and this should get rid of a huge amount of special casing / subclassing in buildbot.

I suspect the python script could be made more robust, but it works in its current form.
Attachment #531367 - Flags: review?(mbanner)
Attachment #531367 - Flags: review?(bugspam.Callek)
Comment on attachment 531367 [details] [diff] [review]
teach comm-central client.mk to invoke client.py

I'm 110% for having a way to run client.py // pass args to client.py but I don't think we should run it always run client.mk is run (even if CLIENT_PY_ARGS is present).

What I suggest/would like is:

CLIENT_PY_ARGS
and
ALWAYS_RUN_CLIENT_PY

for mozconfig (mk_add_options).

Then, [re]add a checkout/co option to client.mk and let the regular usage of mk_add_options call this, to avoid the odd py scriprt and odd run_for_effects usage there.

You can use make syntax to define a target (early in file) for co, and/or write it as a dep for every other rule, etc.

If this is blocking some impressive buildbot work that I don't know about, and you don't have time to do it the right way, I might be able to be convinced though.
Attachment #531367 - Flags: feedback-
v2 of my previous patch. Adds support for ALWAYS_RUN_CLIENT_PY (defaults to false) and introduces a 'co' target for the makefile, so:

make -f client.mk [...]
# Will only run client.py if ALWAYS_RUN_CLIENT_PY is set

make -f client.mk co
# Will *always* run client.py with CLIENT_PY_ARGS
Attachment #531367 - Attachment is obsolete: true
Attachment #531367 - Flags: review?(mbanner)
Attachment #531367 - Flags: review?(bugspam.Callek)
Attachment #531685 - Flags: review?(mbanner)
Attachment #531685 - Flags: review?(bugspam.Callek)
Comment on attachment 531685 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v2

Ok, this looks good by code inspection. r+ if you test on at least Unix and Win32. Windows we need tested with GNUMake (MozBuild) and with Pymake (I *doubt* itd make a difference, but I don't want to regress)

My one nit that I need addressed is that I want to be able to specify client.mk checkout not just co.

The reason I f-'ed is that what I had meant was to drop your mozconfig regex script here and instead use the real mozconfig stuff and let client.mk do all the logic necessary.

But as you mentioned on IRC mozconfig2client-mk etc. you did not feel like porting, but I didn't realize that porting those was necessary for the dropping of that script. I still want it, [and like it 110% better] but won't block your work on that fact.
Attachment #531685 - Flags: review?(bugspam.Callek)
Attachment #531685 - Flags: review+
Attachment #531685 - Flags: feedback-
Assignee: nobody → gozer
Comment on attachment 531685 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v2

>+mozconfig = os.path.join(topsrcdir, os.environ.get('MOZCONFIG','.mozconfig'))

Will this work properly if MOZCONFIG isn't relative (as per my setup?).
(In reply to comment #4)
> Comment on attachment 531685 [details] [diff] [review] [review]
> teach comm-central client.mk to invoke client.py v2
> 
> >+mozconfig = os.path.join(topsrcdir, os.environ.get('MOZCONFIG','.mozconfig'))
> 
> Will this work properly if MOZCONFIG isn't relative (as per my setup?).

As in MOZCONFIG=/path/to/some/mozconfig ? Nope, needs fixing.
(In reply to comment #3)
> Comment on attachment 531685 [details] [diff] [review] [review]
> teach comm-central client.mk to invoke client.py v2
> 
> Ok, this looks good by code inspection. r+ if you test on at least Unix and
> Win32. Windows we need tested with GNUMake (MozBuild) and with Pymake (I
> *doubt* itd make a difference, but I don't want to regress)

It's been running in our staging environment for a few days now, and works on all platforms. Didn't try pymake, however.

> My one nit that I need addressed is that I want to be able to specify
> client.mk checkout not just co.

Simple to fix.

> The reason I f-'ed is that what I had meant was to drop your mozconfig regex
> script here and instead use the real mozconfig stuff and let client.mk do
> all the logic necessary.
> 
> But as you mentioned on IRC mozconfig2client-mk etc. you did not feel like
> porting, but I didn't realize that porting those was necessary for the
> dropping of that script. I still want it, [and like it 110% better] but
> won't block your work on that fact.

I agree, it doesn't make a lot of sense to try and parse that stuff directly, and I am aware that the regexp method is also probably not 100% robust.

However, not sure how to best 'port' the mozilla mozconfig2client-mk stuff, short of just copying it into comm-central and keeping it in sync from time to time. Sounds like a brittle way to port something, IMO. Not sure what other options there are, however.
Comment on attachment 531685 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v2

Please fix the absolute path case then ;-)
Attachment #531685 - Flags: review?(mbanner) → review-
(In reply to comment #3)
> My one nit that I need addressed is that I want to be able to specify
> client.mk checkout not just co.

Yes, please. As module owner for this build system, I'd like to request that to be done in the patch that's going to land, as "make -f client.mk checkout" was the way to do this in pre-hg times and when we re-enable the functionality, it makes sense to also re-enable the syntax. ;-)
This patch includes both requests for improvements:

- make co/checkout
- MOZCONFIG is treated as an absolute path to a mozconfig file
Attachment #533006 - Flags: review?(mbanner)
Attachment #533006 - Flags: review?(bugspam.Callek)
Fix a small bug where we ended up invoking client.py twice, since client.mk somehow calls itself twice.
Attachment #531685 - Attachment is obsolete: true
Attachment #533006 - Attachment is obsolete: true
Attachment #533006 - Flags: review?(mbanner)
Attachment #533006 - Flags: review?(bugspam.Callek)
Attachment #536700 - Flags: review?(mbanner)
Attachment #536700 - Flags: review?(bugspam.Callek)
Comment on attachment 536700 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v4

By inspection this looks fine.
Attachment #536700 - Flags: review?(mbanner) → review+
This latest patch doesn't try and *parse* .mozconfig in python, but rather lets the shell itself execute/parse it.

This was an issue when I tried in try and the .mozconfig included mozconfig-extras, and my parser didn't notice.

This is now a lot less hacky now.
Attachment #536700 - Attachment is obsolete: true
Attachment #536700 - Flags: review?(bugspam.Callek)
changeset:   7980:0f79491664e9
tag:         tip
user:        Philippe M. Chiasson <gozer@mozilla.com>
date:        Mon Jun 20 16:01:36 2011 -0400
summary:     Bug 656049 - teach comm-central client.mk to invoke client.py. r=Standard8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
This breaks my windows build environment on comm-central (sorry for the German error messages, but do not know how to change):
"Mozilla tools directory: c:\Users\joachim\mozilla-build\"
Windows SDK directory: C:\Program Files\Microsoft SDKs\Windows\v7.0\
Windows SDK version: 0.0
Platform SDK directory: C:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\
Platform SDK version: 5
Setting environment for using Microsoft Visual Studio 2008 x86 tools.
Mozilla build environment: MSVC version 9.

hg update -r 7980
$ make  -f client.mk build
Traceback (most recent call last):
  File "c:/Users/joachim/workspace/comm/build/run_client.py", line 21, in <module>
    process = Popen(['build/print_mk_add_options.sh'], stdout=PIPE, stderr=sys.stderr)
  File "c:\Users\joachim\mozilla-build\python\lib\subprocess.py", line 633, in __init__
    errread, errwrite)
  File "c:\Users\joachim\mozilla-build\python\lib\subprocess.py", line 842, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 ist keine zulõssige Win32-Anwendung
Adding client.mk options from /c/Users/joachim/workspace/comm/.mozconfig:
    MOZ_OBJDIR=$(TOPSRCDIR)/obj-tb
    MOZ_CO_PROJECT=mail
Traceback (most recent call last):
  File "c:/Users/joachim/workspace/comm/build/run_client.py", line 21, in <module>
    process = Popen(['build/print_mk_add_options.sh'], stdout=PIPE, stderr=sys.stderr)
  File "c:\Users\joachim\mozilla-build\python\lib\subprocess.py", line 633, in __init__
    errread, errwrite)
  File "c:\Users\joachim\mozilla-build\python\lib\subprocess.py", line 842, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 ist keine zulõssige Win32-Anwendung
make[1]: Entering directory `/c/Users/joachim/workspace/comm'
cd /c/Users/joachim/workspace/comm/obj-tb
/c/Users/joachim/workspace/comm/configure
...
checking for minimum required Python version >= 2.5... yes
/c/Users/joachim/workspace/comm/configure: line 6319: syntax error near unexpected token `lto_is_enabled'
/c/Users/joachim/workspace/comm/configure: line 6319: `MOZ_DOING_LTO(lto_is_enabled)'
*** Fix above errors and then restart with               "make -f client.mk build"
make[1]: *** [configure] Error 1
make[1]: Leaving directory `/c/Users/joachim/workspace/comm'
make: *** [/c/Users/joachim/workspace/comm/obj-tb/Makefile] Error 2

hg update -r 7979 does not show this problem.
An update: With the mozilla-central branch the build works, with http://hg.mozilla.org/releases/mozilla-2.0 (which seems to be the default when you call client.py checkout) the build fails.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
@herb, can you try with this patch on top of comm-central and see if it makes your errors disappear ?
Attachment #540564 - Attachment is obsolete: true
Attachment #540867 - Flags: review?(mbanner)
(In reply to comment #16)
> @herb, can you try with this patch on top of comm-central and see if it
> makes your errors disappear ?
The call to configure worked and compilation has started. So the patch seems to solve the problem.
I'm getting these errors:

$ make -f client.mk
Traceback (most recent call last):
  File "c:/t1/hg/comm-central/build/run_client.py", line 21, in <module>
    process = Popen(['build/print_mk_add_options.sh'], stdout=PIPE, stderr=sys.stderr)
  File "c:\DEV\mozilla-build\python\lib\subprocess.py", line 633, in __init__
    errread, errwrite)
  File "c:\DEV\mozilla-build\python\lib\subprocess.py", line 842, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 is not a valid Win32 application
Adding client.mk options from /c/t1/hg/comm-central/mozconfig:
    MOZ_CO_PROJECT=suite
    MOZ_OBJDIR=$(TOPSRCDIR)/../objdir-sm
    MOZ_MAKE_FLAGS=-j2
Generating /c/t1/hg/comm-central/mozilla/configure using autoconf
This is probably my final version for now. It's much cleaner, and also avoids double-running when we have to clone mozilla/ from an initial checkout.

Try this patch, should work on windows fine too.
Attachment #540867 - Attachment is obsolete: true
Attachment #540867 - Flags: review?(mbanner)
Attachment #541128 - Flags: review?(mbanner)
Justin: Suggest we land gozer's patch now and refactor it later if necessary so we can continue to make progress on getting more in sync with the Firefox team's automation.  Any objections?
Comment on attachment 541128 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v5.2

>diff --git a/build/run_client.py b/build/run_client.py
>-process = Popen(['build/print_mk_add_options.sh'], stdout=PIPE, stderr=sys.stderr)
>+process = Popen([options.make,'-f', 'client.mk', 'print_mk_add_options', 'NO_CLIENT_PY=1'], stdout=PIPE, stderr=sys.stderr)

Lets just say, this sort of scares me, client.mk->run_client.py->client.mk->print_mk_add_options->[process] is a sort of scary chain that is not too apparant

That said, I don't see an easy way around this other than my (overdue) promised edit to this all.

The perfect is the enemy of the "works", not this time!
Attachment #541128 - Flags: review?(mbanner) → review+
(In reply to comment #20)
> Justin: Suggest we land gozer's patch now and refactor it later if necessary
> so we can continue to make progress on getting more in sync with the Firefox
> team's automation.  Any objections?

Did not test locally, but looks like it would fix those above errors, so r+ for landing since it blocks you, and my time is sadly not open to finish my promised work here.
P.S. I would appreciate this pass try before going live.

Also this should bake on trunk, for at least a week before it gets landed on -aurora/-beta imo, so we can let the varying types of dev setups QA this patch, so we don't break people on aurora with this. Assuming no complaints in that week we can take it on aurora/beta imo.
(In reply to comment #23)
> P.S. I would appreciate this pass try before going live.

Passed on try for all platforms.
Landed on comm-central in this changeset:

changeset:   8069:10df3ade195d
parent:      7991:ec4a75961dc8
user:        Philippe M. Chiasson <gozer@mozilla.com>
date:        Thu Jul 07 13:03:58 2011 -0400
summary:     Backed out changeset ec4a75961dc8

changeset:   8070:31cb62010d42
parent:      8068:a6bdbe78f1f7
parent:      8069:10df3ade195d
user:        Philippe M. Chiasson <gozer@mozilla.com>
date:        Thu Jul 07 13:04:03 2011 -0400
summary:     merge

changeset:   8071:1fe64d255b2b
user:        Philippe M. Chiasson <gozer@mozilla.com>
date:        Thu Jul 07 13:04:40 2011 -0400
summary:     Bug 656049 - teach comm-central client.mk to invoke client.py r=Standard8,Callek
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Now getting an infinite loop when trying to build:

$ make -f client.mk
/c/t1/hg/comm-central/build/print_mk_add_options.sh: line 18: ./.mozconfig: No such file or directory
make[1]: *** [print_mk_add_options] Error 1
/c/t1/hg/comm-central/mozilla/build/autoconf/mozconfig2client-mk /c/t1/hg/comm-central /c/t1/hg/comm-central/.mozconfig.mk > /c/t1/hg/comm-central/.mozconfig.out
/c/t1/hg/comm-central/build/print_mk_add_options.sh: line 18: ./.mozconfig: No such file or directory
make[1]: *** [print_mk_add_options] Error 1
/c/t1/hg/comm-central/mozilla/build/autoconf/mozconfig2client-mk /c/t1/hg/comm-central /c/t1/hg/comm-central/.mozconfig.mk > /c/t1/hg/comm-central/.mozconfig.out
/c/t1/hg/comm-central/build/print_mk_add_options.sh: line 18: ./.mozconfig: No such file or directory
make[1]: *** [print_mk_add_options] Error 1
/c/t1/hg/comm-central/mozilla/build/autoconf/mozconfig2client-mk /c/t1/hg/comm-central /c/t1/hg/comm-central/.mozconfig.mk > /c/t1/hg/comm-central/.mozconfig.out
Traceback (most recent call last):
  File "c:/t1/hg/comm-central/build/run_client.py", line 38, in <module>
    output, unused_err = process.communicate()
  File "c:\DEV\mozilla-build\python\lib\subprocess.py", line 693, in communicate
    stdout = self.stdout.read()
KeyboardInterrupt
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Does not seem to work with pymake, whose make "program" includes arguments.
(I was able to get my build working by specifying NO_CLIENT_PY=1.)

Note that the old CVS client.mk checked out as a default action or with an explicit "checkout", but did not check out for just "configure" or "build".
Not working for me either with pymake:

$ pymake -f ../client.mk -sj4
c:\builds\acland\client.mk:142:0:Line expands to non-empty value
gozer, if you could figure out how to make this work with pymake, it would be much appreicated. I tried adding mk_add_options NO_CLIENT_PY=1 to my mozconfig and I got the same error.
(In reply to comment #28)
> Not working for me either with pymake:

Ok, since pymake is a great tool for windows users, and many people use it, lets backout (again). My idea might work, but needs me to have time to write it. Otherwise gozer can try again :-)
No-one backed out yet, so I did (as one clean rev, rather than the backout-backout-of-backout+backout-merge+backout-changes+merge-all-those-backouts.

http://hg.mozilla.org/comm-central/rev/f78f865e5fad
(In reply to comment #31)
> No-one backed out yet, so I did (as one clean rev, rather than the
> backout-backout-of-backout+backout-merge+backout-changes+merge-all-those-
> backouts.
> 
> http://hg.mozilla.org/comm-central/rev/f78f865e5fad

err + 
http://hg.mozilla.org/comm-central/rev/004c3d95bb3e
Heres the first part of my idea here. Basically copy the mozilla/* dependancies that are sourced early to c-c.

A followup bug will be adding tests that the files don't diverge, (and if they do we'll turn orange).

This will allow us to (always) find the .mozconfig properly, and actually be able to invoke client.py without having to have those weird wrapper scripts and we can do all the work inside client.mk easily.

ETA on next patch is probably sometime next week, (I'll review for you if you want to build on top of this patch instead)
Attachment #545604 - Flags: review?(mbanner)
Attachment #545604 - Flags: review?(gozer)
Attachment #545604 - Flags: review?(mbanner)
Attachment #545604 - Flags: review?(gozer)
Attachment #545604 - Flags: review+
Callek: When can we move this forward? My understanding is that this is blocking a major update to our buildbot that we really need.
(In reply to Justin Wood (:Callek) from comment #33)
> Created attachment 545604 [details] [diff] [review]
> Do the first part
> 
> Heres the first part of my idea here. Basically copy the mozilla/*
> dependancies that are sourced early to c-c.

Careful, trying this on Linux failed because I didn't set mozconfig2client-mk config.guess mozconfig-find to be executable.
Landed Callek's patch in https://hg.mozilla.org/comm-central/rev/088aec7c13fb after setting these execute permissions:

$ chmod a+x build/autoconf/mozconfig2client-mk build/autoconf/config.guess build/autoconf/mozconfig-find
This new patch is so much simpler, now that attachment 545604 [details] [diff] [review] landed.
Attachment #541128 - Attachment is obsolete: true
Attachment #556105 - Flags: review?(mbanner)
Attachment #556105 - Flags: review?(bugspam.Callek)
Comment on attachment 556105 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v6

>+# These targets are candidates for auto-running client.py
>+ifdef  ALWAYS_RUN_CLIENT_PY
>+$(TOPSRCDIR)/.mozconfig.mk:: run_client_py

This looks a bit suspect, why are we run_client_py setting a dep against .mozconfig.mk ?

Nothing that depends on anything outside of c-c is creating .mozconfig.mk and we $(shell ...) the generation of it, and include it above. iirc we do hit this dep run when we include that, but I don't think we should be running the dep here.

If there is some particular issue you are fixing by that, I'll entertain it.

Also, can you please file a followup on me, for re-syncing the 4 files I patched in the previous patch, and getting a |make check| test checked in.
Attachment #556105 - Flags: review?(bugspam.Callek) → review-
(In reply to Justin Wood (:Callek) from comment #38)
> Comment on attachment 556105 [details] [diff] [review]
> teach comm-central client.mk to invoke client.py v6
> 
> >+# These targets are candidates for auto-running client.py
> >+ifdef  ALWAYS_RUN_CLIENT_PY
> >+$(TOPSRCDIR)/.mozconfig.mk:: run_client_py
> 
> This looks a bit suspect, why are we run_client_py setting a dep against
> .mozconfig.mk ?

Leftover from my previous, more complex patch. Removed.
 
> Also, can you please file a followup on me, for re-syncing the 4 files I
> patched in the previous patch, and getting a |make check| test checked in.

Bug 682897 & bug 682898
Attachment #556105 - Attachment is obsolete: true
Attachment #556105 - Flags: review?(mbanner)
Attachment #556607 - Flags: review?(mbanner)
Attachment #556607 - Flags: review?(bugspam.Callek)
Comment on attachment 556607 [details] [diff] [review]
teach comm-central client.mk to invoke client.py v7

Much much better than earlier incarnations. Sorry it took me a while to tackle.
Attachment #556607 - Flags: review?(mbanner)
Attachment #556607 - Flags: review?(bugspam.Callek)
Attachment #556607 - Flags: review+
Blocks: 682078
Attachment #556607 - Flags: review?(mbanner)
Attachment #556607 - Flags: review?(mbanner) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/9e778f4626d4
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
Followup patch. Turns out that client.mk sometimes calls itself recursively (i.e. $(MAKE) client.mk configure)) and that causes client.py to be invoked
multiple times.

Not terrible in itself, but wasteful and causing duplicate tinderboxprints.

This simple patch makes sure client.py is only invoked once.
Attachment #557551 - Flags: review?(mbanner)
Comment on attachment 557551 [details] [diff] [review]
Make sure to run client.py only once per make invocation

Presuming that you want it to run in _top_ level make, r-.

You can have implied r+ if you change it to 0 per the GNU Make documentation, I've attached below.

--
As a special feature, the variable MAKELEVEL is changed when it is passed down from level to level. This variable's value is a string which is the depth of the level as a decimal number. The value is ‘0’ for the top-level make; ‘1’ for a sub-make, ‘2’ for a sub-sub-make, and so on. The incrementation happens when make sets up the environment for a recipe. 
--
Attachment #557551 - Flags: review?(mbanner) → review-
(In reply to Justin Wood (:Callek) from comment #43)
> Comment on attachment 557551 [details] [diff] [review]
> Make sure to run client.py only once per make invocation
> 
> Presuming that you want it to run in _top_ level make, r-.

Yes, my bad. I was testing under osx universal build, where things get invoked recursively quite a bit. Verified that 0 is quite correctly the right level to test for.

> You can have implied r+ if you change it to 0 per the GNU Make
> documentation, I've attached below.

Excellent, will land shortly.
changeset:   8412:9dbeb66df9ab
tag:         tip
user:        Philippe M. Chiasson <gozer@mozilla.com>
date:        Thu Sep 01 15:04:26 2011 -0400
summary:     Bug 656049 - Make sure to run client.py only once per make invocation r=Callek
You need to log in before you can comment on or make changes to this bug.