Closed Bug 548207 Opened 14 years ago Closed 14 years ago

allow make check to run over remote connection from devicemanager.py

Categories

(Firefox Build System :: General, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: blassey, Assigned: blassey)

Details

Attachments

(3 files, 8 obsolete files)

23.30 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
1.85 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.79 KB, patch
Callek
: review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — Splinter Review
      No description provided.
Attachment #428640 - Flags: review?(ctalbert)
is there anyway we can avoid hardcoding the host?  maybe pass this in via the make command line:
make HOST=192.168.1.201 check

Another think to consider is this requires a device, but is integrated in the build system.  Will the winmo builds from buildbot try to run this script?  Should this script copy over the package to a device if available?
Comment on attachment 428640 [details] [diff] [review]
patch

>+ifeq (WINCE,$(OS_ARCH))
>+RUN_TEST_PROGRAM = $(PYTHON) $(topsrcdir)/build/devicemanager-run-test.py
>+endif
>+

I was just thinking, maybe this should be conditional on CROSS_COMPILE
that is probably the right thing to do.  We plan on using the same toolset for android, so this would just fit right in.  And if we made a test-agent for maemo, this would work the same way :)
Attached patch patch v.2 (obsolete) — Splinter Review
this also adds devicemanager-utils.py which it uses in xpcom/tests/Makefile.in to run the TestRegistrationOrder test.  I'm sure the changes to xpcom/tests/Makefile.in could be cleaner, suggestions welcomed.
Assignee: nobody → bugmail
Attachment #428640 - Attachment is obsolete: true
Attachment #428894 - Flags: review?(ctalbert)
Attachment #428640 - Flags: review?(ctalbert)
I like the concept of devicemanager-utils.py.  I don't know if it should live in the build directory though.  I am still waiting to see the DEVICEIP or HOST variable added dynamically.  

Another option for dm-utils would be install which would copy and then unpack the file on the device.

so then we could have:
<psuedo.mk>
HOST = a.b.c.d
check : install
 python command device=@HOST

install : package
 copy device=@HOST dist/fennec*.exe /tests/fennec.exe
 unpack device=@HOST /tests/fennec.exe

package : #essentially make package
</psuedo.mk>

I would like to avoid  hard coded file names, and also avoid adding a lot of stuff specific to this in the existing makefiles which Firefox uses.  If there is a winmo specific makefile that we gets included for cross-compiling, then I would prefer we add a lot of changes there.

On the same note, is it possible to put some of this stuff in build/wince/remote instead of the root build directory?  I know these tools will be useful for android, but right now we are using them for winmo only.
Brad, I like what you're doing here.  I'd love to see the compiled code tests run on the devices.  I have all the same concerns about hard-coded IP's, files etc that Joel brings up in comment 5.  But before I can really review this I have a larger question here: Do you intend for these to be run via buildbot or by hand from developers?  

That's important, because all the buildbot integration stuff we've worked out with releng has centered around the "packaged tests" method and that excludes the compiled code tests.  Currently compiled code tests are run directly on the machines that build the code as a post-build step.  In the current "mobile-buildbot world", the machines that do the build will not have any way to know what devices are available, what device IPs there are etc.  I think if that is our intention then we need to think about a way to ensure those tests are compiled staticly and are packaged up via package-tests so that they will be available along with our other tests.

Another option would be to try to convince RelEng to make the builder more aware that during a "remoteable" build (i.e. maemo, fennec, android etc) it has a DeviceIP and then as its post build step it could seed your make file logic with that IP and run the tests that way.  I have no idea how difficult that would be to hack into buildbot at the moment.

If you aren't that interested in buildbot integration and you intend to give developers an easy way to run these tests on the devices, then I think you're on the right track here with these patches, we just have to make sure that the buildbot machines have a graceful failure path when they find that they don't have a deviceIP to talk to - I think that can probably be done inside your python classes.

I think solving both the "developer use case" and the "buildbot use case" are extremely valuable here, so I'm all for these changes.  But if we want to solve the buildbot case, then this problem is a bit bigger than it might at first seem.
we chatting with Aki regarding make check and winmo and it is not being done and there are no plans to do it.  So the buildbot case is not intended.  The one thing we should safe guard is can we provide a realistic error message in case it is accidentally run.
My goal was only to allow an individual to run the native tests, although anything that gets allows an individual to run them obviously makes it easier for a buildbot to as well. 

My understanding from aki is that only a subset of buildbots that do debug builds run make check, so I'm of the opinion that we can cross the "run native tests on buildbot" bridge if we ever get to it. We don't have any debug build bots for winmo or maemo at this time.

For the device's ip address and port, I'd be interested to know how you intend to configure that for unit tests and talos so we can be consistent.

As far as Joel's concern of polluting makefile, there is no makefile that only gets run for cross compiles.  All makefiles should handle being run as a cross compile (bsdiff didn't and look at the pain that caused coop). My only concern with the makefiles is xpcom/tests/Makefile.in because my changes there are a little ugly, but then again the check target in that file started out ugly.
Attached patch patch v.3 (obsolete) — Splinter Review
this looks for 3 env vars to get the right parameters (DEVICE_IP, DEVICE_PORT and REMOTE_GRE_PATH).  It also copies the build in the top level check target.


I now get a full successful run of make check on the omnia II.
Attachment #428894 - Attachment is obsolete: true
Attachment #429211 - Flags: review?(ctalbert)
Attachment #428894 - Flags: review?(ctalbert)
Comment on attachment 429211 [details] [diff] [review]
patch v.3

Hey Brad, these changes look good.  I have one question:

I don't see where you are setting "DEVICE_IP", "DEVICE_PORT", or "REMOTE_GRE_PATH".  Are you intending to do that by hand?

Otherwise, these changes look good.
Attachment #429211 - Flags: review?(ctalbert) → review+
they're environmental variables that need to be set
Attachment #429211 - Flags: review?(ted.mielczarek)
ok cool.  Thanks for this patch, blassey
Comment on attachment 429211 [details] [diff] [review]
patch v.3

>diff --git a/Makefile.in b/Makefile.in
>--- a/Makefile.in
>+++ b/Makefile.in
>@@ -79,24 +79,29 @@ ifdef ENABLE_TESTS
> tier_testharness_dirs += testing/xpcshell
> endif
> 
> include $(topsrcdir)/config/config.mk
> 
> GARBAGE_DIRS += dist _javagen _profile _tests staticlib
> DIST_GARBAGE = config.cache config.log config.status config-defs.h \
>    dependencies.beos config/autoconf.mk \
>    unallmakefiles mozilla-config.h \
>    netwerk/necko-config.h xpcom/xpcom-config.h xpcom/xpcom-private.h \
>    $(topsrcdir)/.mozconfig.mk $(topsrcdir)/.mozconfig.out
> 
>+ifdef CROSS_COMPILE
>+check::
>+	$(PYTHON) $(topsrcdir)/build/devicemanager-utils.py copy $(DIST)/bin
>+endif

I think you should either put a test inside this block for the environment variables that your devicemanager scripts expect, or put an error error in this devicemanager-utils script so you can error early with a useful error message. I guess the latter is preferable since the script won't work without them anyway.

>diff --git a/build/devicemanager-run-test.py b/build/devicemanager-run-test.py
>new file mode 100644
>--- /dev/null
>+++ b/build/devicemanager-run-test.py

Can you stick these files in build/mobile/ or something? build/ is already kind of cluttered.

You should also slap a license header on each of these scripts.

>@@ -0,0 +1,30 @@
>+import devicemanager
>+import sys
>+import os
>+
>+ip_addr = os.environ.get("DEVICE_IP")
>+port = os.environ.get("DEVICE_PORT")
>+gre_path = os.environ.get("REMOTE_GRE_PATH").replace('\\','/')

You should error with a useful error message if these aren't set.

>+
>+dm = devicemanager.DeviceManager(ip_addr, int(port))
>+cmd = sys.argv[1]
>+args = ' '
>+args= ' ' + args.join(sys.argv[2:])

You don't actually check len(sys.argv) here, you should. Also, this is a really weird way of writing this, why not just:
args = ' ' + ' '.join(sys.argv[2:])

>+dm.debug = 0
>+lastslash = cmd.rfind('/');
>+if (lastslash == -1):

You don't need the parens in python if statements.

>+    lastslash = 0
>+dm.pushFile(cmd, gre_path + cmd[lastslash:])
>+process = dm.launchProcess([gre_path  + cmd[lastslash:] + args])
>+output_list = dm.communicate(process)
>+ret = -1
>+if (output_list != None and output_list[0] != None):
>+    try:
>+        output = output_list[0]
>+        index = output.find('exited with return code')
>+        print output[0:index]
>+        retstr = (output[index + 24 : output.find('\n', index)])
>+        ret = int(retstr)
>+    except ValueError:
>+        ret = -1
>+sys.exit(ret)

Finally, I prefer that all our Python scripts define a top-level function, and invoke that, so that we could potentially load them as modules if need be. Something like:
def main():
 ...

if __name__ == '__main__':
  main()

>diff --git a/build/devicemanager-utils.py b/build/devicemanager-utils.py
>new file mode 100644
>--- /dev/null
>+++ b/build/devicemanager-utils.py

Same thing here, license header + main function.

>@@ -0,0 +1,35 @@
>+import devicemanager
>+import sys
>+import os
>+
>+ip_addr = os.environ.get("DEVICE_IP")
>+port = os.environ.get("DEVICE_PORT")
>+gre_path = os.environ.get("REMOTE_GRE_PATH").replace('\\','/')

And an error message for the env vars.

>+
>+dm = devicemanager.DeviceManager(ip_addr, int(port))
>+dm.sendCMD(['cd '+ gre_path], sleep = 1)
>+dm.debug = 0
>+
>+def copy():
>+    file = sys.argv[2]

You need to check len(sys.argv) here.

>+    if len(sys.argv) >= 4:
>+        path = sys.argv[3]
>+    else:
>+        path = gre_path
>+    if os.path.isdir(file):
>+        dm.pushDir(file, path.replace('\\','/'))
>+    else:
>+        dm.pushFile(file, path.replace('\\','/'))
>+
>+def delete():
>+    file = sys.argv[2]
>+    dm.removeFile(file)
>+
>+
>+cmd = sys.argv[1]

You need to check len(sys.argv) here also.

>+if (cmd == 'copy'):
>+    sys.exit(copy())
>+if (cmd == 'delete'):
>+    sys.exit(delete())
>+sys.exit(-1)

You should have an error message for unknown commands.

>diff --git a/xpcom/tests/Makefile.in b/xpcom/tests/Makefile.in
>--- a/xpcom/tests/Makefile.in
>+++ b/xpcom/tests/Makefile.in
>@@ -181,27 +181,40 @@ export::
> install::
> 	$(SYSINSTALL) $(IFLAGS1) $(srcdir)/test.properties $(DESTDIR)$(mozappdir)/res
> 
> ifeq (,$(filter-out WINNT WINCE os2-emx, $(HOST_OS_ARCH)))
> swapslashes = $(shell echo $(1) | sed -e 's|/|\\|g')
> getnativepath = $(call swapslashes,$(call normalizepath,$(1)))
> else
> getnativepath = $(1)
> endif
> 
> abs_srcdir = $(shell cd $(srcdir) && pwd)
> 
>+DM_RM = $(PYTHON) $(topsrcdir)/build/devicemanager-utils.py delete
>+DM_CP = $(PYTHON) $(topsrcdir)/build/devicemanager-utils.py copy

Just move these inside the ifdef CROSS_COMPILE

>+
>+ifdef CROSS_COMPILE
>+RM_DIST = $(DM_RM)
>+regOrderDir=\\tests\\regorder;

This looks very wince-specific.

r=me with those things fixed.

This is pretty cool overall!
Attachment #429211 - Flags: review?(ted.mielczarek) → review+
Attached patch patch with ted's nits addressed (obsolete) — Splinter Review
Attachment #429211 - Attachment is obsolete: true
Attachment #431384 - Flags: review?(jmaher)
Attachment #431384 - Attachment is obsolete: true
Attachment #431385 - Flags: review?(jmaher)
Attachment #431384 - Flags: review?(jmaher)
Comment on attachment 431385 [details] [diff] [review]
patch with ted's nits addressed v.2


>diff --git a/Makefile.in b/Makefile.in
>+ifdef CROSS_COMPILE
>+check::
>+	$(PYTHON) $(topsrcdir)/build/mobile/devicemanager-utils.py copy $(DIST)/bin
>+endif
>+

will this copy everything recursively in $(DIST)/bin, and is that what we really want?  I assume so

>diff --git a/build/mobile/devicemanager-run-test.py b/build/mobile/devicemanager-run-test.py
>+    cmd = sys.argv[1]
>+    args = ' '
>+    if len(sys.argv > 2):
>+        args = ' ' + ' '.join(sys.argv[2:])
>+

Please fix the paren in 'len(sys.argv > 2)' to be 'len(sys.argv) > 2'.
What if there are 2 args?  

>+    dm.pushFile(cmd, gre_path + cmd[lastslash:])
>+    process = dm.launchProcess([gre_path  + cmd[lastslash:] + args])

Could you add a comment outlining what command is executing (maybe an example).  I am concerned about a missing slash, space, or a list inside a list.

>diff --git a/build/mobile/devicemanager-utils.py b/build/mobile/devicemanager-utils.py
>+def copy(dm, gre_path):
>+    if os.path.isdir(file):
>+        dm.pushDir(file, path.replace('\\','/'))
>+    else:
>+        dm.pushFile(file, path.replace('\\','/'))
>+

could you define file in here?  I assume it is something to pass on the cli or should it be path?

>+def delete(dm, gre_path):
>+    file = sys.argv[2]
>+    dm.removeFile(file)
>+

same here, gre_path | file?

>+def main():
>+
>+    dm = devicemanager.DeviceManager(ip_addr, int(port))
>+    dm.sendCMD(['cd '+ gre_path], sleep = 1)
>+    dm.debug = 0

what if gre_path doesn't exist?  should we makeDir on it first?

>diff --git a/build/devicemanager.py b/build/mobile/devicemanager.py
>rename from build/devicemanager.py
>rename to build/mobile/devicemanager.py

moving devicemanager.py will break other makefiles that depend on it in build/.  Please see the original attachment for devicemanager.py: https://bug541410.bugzilla.mozilla.org/attachment.cgi?id=428356.

specifically these files:
/testing/mochitest/Makefile.in
/testing/xpcshell/Makefile.in
/layout/tools/reftest/Makefile.in


r- for the devicemanager.py stuff, other stuff are just nits.
Attachment #431385 - Flags: review?(jmaher) → review-
(In reply to comment #16)
> (From update of attachment 431385 [details] [diff] [review])
> 
> >diff --git a/Makefile.in b/Makefile.in
> >+ifdef CROSS_COMPILE
> >+check::
> >+	$(PYTHON) $(topsrcdir)/build/mobile/devicemanager-utils.py copy $(DIST)/bin
> >+endif
> >+
> 
> will this copy everything recursively in $(DIST)/bin, and is that what we
> really want?  I assume so
yes and yes
> 
> >diff --git a/build/mobile/devicemanager-run-test.py b/build/mobile/devicemanager-run-test.py
> >+    cmd = sys.argv[1]
> >+    args = ' '
> >+    if len(sys.argv > 2):
> >+        args = ' ' + ' '.join(sys.argv[2:])
> >+
> 
> Please fix the paren in 'len(sys.argv > 2)' to be 'len(sys.argv) > 2'.
> What if there are 2 args?  
if there are 2 args, we just have a command with no arguments so args will be ' '
 
> >+    dm.pushFile(cmd, gre_path + cmd[lastslash:])
> >+    process = dm.launchProcess([gre_path  + cmd[lastslash:] + args])
> 
> Could you add a comment outlining what command is executing (maybe an example).
>  I am concerned about a missing slash, space, or a list inside a list.
cmd[lastslash:] should be the file name (i.e. fennec.exe). We are relying on gre_path to have a trailing slash.

> 
> >diff --git a/build/mobile/devicemanager-utils.py b/build/mobile/devicemanager-utils.py
> >+def copy(dm, gre_path):
> >+    if os.path.isdir(file):
> >+        dm.pushDir(file, path.replace('\\','/'))
> >+    else:
> >+        dm.pushFile(file, path.replace('\\','/'))
> >+
> 
> could you define file in here?  I assume it is something to pass on the cli or
> should it be path?

file=sys.argv[2], so its the blah.txt in "python devicemanager-utils.py copy blah.txt"
> 
> >+def delete(dm, gre_path):
> >+    file = sys.argv[2]
> >+    dm.removeFile(file)
> >+
> 
> same here, gre_path | file?
file is the same as above. gre_path is taken from the env var
> >+def main():
> >+
> >+    dm = devicemanager.DeviceManager(ip_addr, int(port))
> >+    dm.sendCMD(['cd '+ gre_path], sleep = 1)
> >+    dm.debug = 0
> 
> what if gre_path doesn't exist?  should we makeDir on it first?
I think its okay to assume it exists.  we can add a mkdir command to utils though (follow up)
> 
> >diff --git a/build/devicemanager.py b/build/mobile/devicemanager.py
> >rename from build/devicemanager.py
> >rename to build/mobile/devicemanager.py
> 
> moving devicemanager.py will break other makefiles that depend on it in build/.
>  Please see the original attachment for devicemanager.py:
> https://bug541410.bugzilla.mozilla.org/attachment.cgi?id=428356.
> 
> specifically these files:
> /testing/mochitest/Makefile.in
> /testing/xpcshell/Makefile.in
> /layout/tools/reftest/Makefile.in
> 
> 
> r- for the devicemanager.py stuff, other stuff are just nits.
Comment on attachment 431640 [details] [diff] [review]
patch with ted's nits addressed v.3
[Checkin: See comment 20]

Looks good.
Attachment #431640 - Flags: review?(jmaher) → review+
Comment on attachment 431640 [details] [diff] [review]
patch with ted's nits addressed v.3
[Checkin: See comment 20]


http://hg.mozilla.org/mozilla-central/rev/9a52e9c81b70
+
http://hg.mozilla.org/mozilla-central/rev/ec4a706bfc32
follow up to bug 548207, test for WINCE instead of CROSS_COMPILE due to make check failure on OSX
Attachment #431640 - Attachment description: patch with ted's nits addressed v.3 → patch with ted's nits addressed v.3 [Checkin: See comment 20]
Status: NEW → ASSIGNED
Attachment #432152 - Flags: review?(ted.mielczarek)
Comment on attachment 432149 [details] [diff] [review]
(Cv1-CC) Port (the useful part of) it to comm-central

Mark, do we need to care about TB&1.9.2 with WINCE build-system stuff?

Or should I consider WinCE on 1.9.2 completely unsupported here?
Attachment #432149 - Flags: feedback?(bugzilla)
Attachment #432149 - Attachment is obsolete: true
Attachment #432149 - Flags: review?(bugspam.Callek)
Attachment #432149 - Flags: feedback?(bugzilla)
Comment on attachment 432149 [details] [diff] [review]
(Cv1-CC) Port (the useful part of) it to comm-central


(In reply to comment #23)

Oh! I totally missed the m-1.9.2 part: I'll update this patch after my m-c patch is reviewed ;->
Dv1, with /js/src/ copy.
Attachment #432152 - Attachment is obsolete: true
Attachment #432421 - Flags: review?(ted.mielczarek)
Attachment #432152 - Flags: review?(ted.mielczarek)
Attachment #432421 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 432421 [details] [diff] [review]
(Dv1a) Tidy up RUN_TEST_PROGRAM conditionals
[Checkin: Comment 26]


http://hg.mozilla.org/mozilla-central/rev/88d23abb0c69
Attachment #432421 - Attachment description: (Dv1a) Tidy up RUN_TEST_PROGRAM conditionals → (Dv1a) Tidy up RUN_TEST_PROGRAM conditionals [Checkin: Comment 26]
All pieces together!
Attachment #434412 - Attachment is obsolete: true
Attachment #434421 - Flags: review?(bugspam.Callek)
Attachment #434412 - Flags: review?(bugspam.Callek)
Attachment #434412 - Attachment description: (Ev1-CC) Copy RUN_TEST_PROGRAM part to comm-central → (Ev1-CC) Copy RUN_TEST_PROGRAM part to comm-central [Merged into Cv2-CC]
Attachment #434421 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 434421 [details] [diff] [review]
(Cv2-CC) Port (the useful part of) it to comm-central, (partly m-1.9.2+)
[Checkin: Comment 29]


http://hg.mozilla.org/comm-central/rev/e1441916999b
Attachment #434421 - Attachment description: (Cv2-CC) Port (the useful part of) it to comm-central, (partly m-1.9.2+) → (Cv2-CC) Port (the useful part of) it to comm-central, (partly m-1.9.2+) [Checkin: Comment 29]
can this be closed?
this worked for windows mobile, I don't think there was anything else to do.  We don't support windows mobile anymore officially.  Is there a need for this on Android?  If so that should be in a bug of its own.

I vote for closing this bug.
I wrote patches to do this for android, can't remember the bug number though.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.