1352054 - Support mach awsy-test on Windows

Status: RESOLVED FIXED

(Testing :: AWSY, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: mach awsy-test --quick output on windows

I finally got a windows build environment and encountered errors attempting to run mach awsy-test.

It may be as simple as making sure to use os.path.join when constructing paths. More news soon.

Comment 1

[Eric Rahm [:erahm]]

Basically it's failing to run tooltool:

> 0:00.69 c:/dev/mozilla-central\taskcluster/docker/recipes/tooltool.py --manifest=c:/dev/mozilla-central\testing\awsy\tp5n-pageset.manifest --unpack --cache-folder=c:/dev/mozilla-central\tooltool-cache fetch

Comment 2

[Eric Rahm [:erahm]]

Running by hand seems fine:

> $ /c/dev/mozilla-central\\taskcluster/docker/recipes/tooltool.py --manifest=c:/dev/mozilla-central\\testing\\awsy\\tp5n
-pageset.manifest --unpack --cache-folder=c:/dev/mozilla-central\\tooltool-cache fetch
> INFO - File tp5n.zip not present in local cache folder c:/dev/mozilla-central\tooltool-cache
> INFO - Attempting to fetch from 'https://api.pub.build.mozilla.org/tooltool/'...
> INFO - File tp5n.zip fetched from https://api.pub.build.mozilla.org/tooltool/ as c:\dev\mozilla-central\tmplmycwy
> INFO - File integrity verified, renaming tmplmycwy to tp5n.zip
> INFO - Updating local cache c:/dev/mozilla-central\tooltool-cache...
> INFO - Creating cache in c:/dev/mozilla-central\tooltool-cache...
> INFO - Local cache c:/dev/mozilla-central\tooltool-cache updated with tp5n.zip

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: mach-awsy-windows.patch

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8858133 [details] [diff] [review]
mach-awsy-windows.patch

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

Thanks for fixing this, just a few minor comments.

::: testing/awsy/mach_commands.py
@@ +52,5 @@
>              tests = [os.path.join(self.topsrcdir,
> +                                  "testing",
> +                                  "awsy",
> +                                  "awsy",
> +                                  "test_memory_usage.py")]

Ugh that's unfortunate. I wonder if we should make a constant of:

> os.path.join(self.topsrcdir, 'testing', 'awsy')

and use that here and below. Minor nit I think we want 'single quotes'

@@ -97,5 @@
>          runtime_testvars_file = open(runtime_testvars_path, 'wb')
>          runtime_testvars_file.write(json.dumps(runtime_testvars, indent=2))
>          runtime_testvars_file.close()
>  
> -        if not kwargs['webRootDir']:

Why'd you get rid of this?

@@ -115,5 @@
> -            self.run_process(cwd=page_load_test_dir, **tooltool_args)
> -            tp5nzip = os.path.join(page_load_test_dir, 'tp5n.zip')
> -            tp5nmanifest = os.path.join(page_load_test_dir, 'tp5n', 'tp5n.manifest')
> -            if not os.path.exists(tp5nmanifest):
> -                files = mozfile.extract_zip(tp5nzip, page_load_test_dir)

Does mozfile.extract_zip not work on Windows? We should definitely fix that. Or is it a bug in zipfile itself?

@@ +213,5 @@
> +        with a short path. For example:
> +
> +        --web-root=c:\\\\tmp\\\\html --results=c:\\\\tmp\\\\results
> +
> +        Note that the double backslashes are required.

Would C:/tmp/html work?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8858133 [details] [diff] [review]
mach-awsy-windows.patch

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

just a few little comments/nits.  Glad to see this come along!

::: testing/awsy/mach_commands.py
@@ +112,5 @@
> +                         "recipes",
> +                         "tooltool.py"),
> +            "--manifest=%s" % manifest_file,
> +            "--cache-folder=%s" % os.path.join(self.topsrcdir, "tooltool-cache"),
> +            "fetch"

as :erahm indicated earlier, single quotes- or at least consistent throughout the file, it is mixed right now.

also, I see you removed the --unpack argument, I assume that was intentional.

@@ +213,5 @@
> +        with a short path. For example:
> +
> +        --web-root=c:\\\\tmp\\\\html --results=c:\\\\tmp\\\\results
> +
> +        Note that the double backslashes are required.

if in mozilla-build environment (mingw32) you could do /c/tmp/html, etc.  I assume this will run via mach, so that implies a / environment.

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to Eric Rahm [:erahm] from comment #4)
> Comment on attachment 8858133 [details] [diff] [review]
> mach-awsy-windows.patch
> 
> Review of attachment 8858133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for fixing this, just a few minor comments.
> 
> ::: testing/awsy/mach_commands.py
> @@ +52,5 @@
> >              tests = [os.path.join(self.topsrcdir,
> > +                                  "testing",
> > +                                  "awsy",
> > +                                  "awsy",
> > +                                  "test_memory_usage.py")]
> 
> Ugh that's unfortunate. I wonder if we should make a constant of:
> 
> > os.path.join(self.topsrcdir, 'testing', 'awsy')
> 
> and use that here and below. Minor nit I think we want 'single quotes'
> 

Ok.

> @@ -97,5 @@
> >          runtime_testvars_file = open(runtime_testvars_path, 'wb')
> >          runtime_testvars_file.write(json.dumps(runtime_testvars, indent=2))
> >          runtime_testvars_file.close()
> >  
> > -        if not kwargs['webRootDir']:
> 
> Why'd you get rid of this?
> 

In the case where the test failed on windows using the default web root and results directories, and we need to specify the web root to a shorter path, this conditional prevented the download and installation of the tp5n pages. It doesn't really hurt. We just check the tooltool cache and only unzip if the tp5n.manifest doesn't already exist.

> @@ -115,5 @@
> > -            self.run_process(cwd=page_load_test_dir, **tooltool_args)
> > -            tp5nzip = os.path.join(page_load_test_dir, 'tp5n.zip')
> > -            tp5nmanifest = os.path.join(page_load_test_dir, 'tp5n', 'tp5n.manifest')
> > -            if not os.path.exists(tp5nmanifest):
> > -                files = mozfile.extract_zip(tp5nzip, page_load_test_dir)
> 
> Does mozfile.extract_zip not work on Windows? We should definitely fix that.
> Or is it a bug in zipfile itself?
> 

It craps out more than plain unzip does. Using the default paths, it will only partly unzip the file resulting in a handful of page directories being created. unzip also has problems on my system due to the long path names in the default configuration and ends up failing due to a return code of 2, but at least it completes.

I don't know the root cause but using unzip explicitly seems to be a common pattern in the tree. Installing mozbase in a virutalenv and doing it by hand shows

>>> import mozfile
>>> mozfile.extract_zip('c:/tmp/html/page_load_test/tp5n.zip', 'c:/tmp/out')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\cygwin\mozilla\builds\hg.mozilla.org\mozilla-central\testing\mozbase\
mozfile\mozfile\mozfile.py", line 72, in extract_zip
    _dest = open(filename, 'wb')
IOError: [Errno 22] invalid mode ('wb') or filename: 'c:\\tmp\\out\\tp5n\\uol.co
m.br\\www.uol.com.br\\b\\ss\\uoluolprod,uoltotal\\1\\H.22.1\\?AQB=1&ndh=1&AQE=1'

So, I would assume tp5n's use of unsafe characters in pathnames is probably a place to start.

> @@ +213,5 @@
> > +        with a short path. For example:
> > +
> > +        --web-root=c:\\\\tmp\\\\html --results=c:\\\\tmp\\\\results
> > +
> > +        Note that the double backslashes are required.
> 
> Would C:/tmp/html work?

The issue is os.path.join on windows ends up using backslashes not forward slashes. We end up with paths like c:/tmp/html\\page_load_test_dir\\ etc which cause me troubles. See below.

==

(In reply to Joel Maher ( :jmaher) from comment #5)
> Comment on attachment 8858133 [details] [diff] [review]
> mach-awsy-windows.patch
> 
> Review of attachment 8858133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just a few little comments/nits.  Glad to see this come along!
> 
> ::: testing/awsy/mach_commands.py
> @@ +112,5 @@
> > +                         "recipes",
> > +                         "tooltool.py"),
> > +            "--manifest=%s" % manifest_file,
> > +            "--cache-folder=%s" % os.path.join(self.topsrcdir, "tooltool-cache"),
> > +            "fetch"
> 
> as :erahm indicated earlier, single quotes- or at least consistent
> throughout the file, it is mixed right now.
> 
> also, I see you removed the --unpack argument, I assume that was intentional.
> 

Yes. It does not work nor is it used elsewhere in the tree that I can see. I actually had redundant unpack and mozfile.extract_zip previously.

> @@ +213,5 @@
> > +        with a short path. For example:
> > +
> > +        --web-root=c:\\\\tmp\\\\html --results=c:\\\\tmp\\\\results
> > +
> > +        Note that the double backslashes are required.
> 
> if in mozilla-build environment (mingw32) you could do /c/tmp/html, etc.  I
> assume this will run via mach, so that implies a / environment.

It is.

fails:  ./mach awsy-test --web-root=/c/tmp/html --results=/c/tmp/results --quick
fails:  ./mach awsy-test --web-root=c:/tmp/html --results=c:/tmp/results --quick
passes: ./mach awsy-test --web-root=c:\\tmp\\html --results=c:\\tmp\\results --quick

I considered just defaulting windows' paths to these values and eliminating the possible failure if your paths are too long, but decided it was kind of ham fisted.

I'll fix the quote issue. If you want me to make additional changes, let me know.

Comment 7

[Eric Rahm [:erahm]]

I wonder if we can use the prefix with '\\?\' trick here. Python's |open| seems okay with it, ie:

> f = open(r'\\?\C:\Users\Eric Rahm\test.txt')

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: mach-awsy-windows-v2.patch

Here is what I have now. If we want to tweak the file handling later, lets do it in a follow up. I just want to get something working and get this off of my plate.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8858167 [details] [diff] [review]
mach-awsy-windows-v2.patch

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

thanks bc

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e27cbd5c699
Support mach awsy-test on Windows, r=erahm, jmaher.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e27cbd5c699

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/174b7b906a7c