Closed Bug 1352054 Opened 2 years ago Closed 2 years ago

Support mach awsy-test on Windows

Categories

(Testing :: AWSY, defect)

defect
Not set

Tracking

(firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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
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
Attached patch mach-awsy-windows.patch (obsolete) — Splinter Review
Attachment #8858133 - Flags: review?(jmaher)
Attachment #8858133 - Flags: review?(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?
Attachment #8858133 - Flags: review?(erahm) → review+
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.
Attachment #8858133 - Flags: review?(jmaher) → review+
(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.
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')
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.
Attachment #8858133 - Attachment is obsolete: true
Attachment #8858167 - Flags: review?(jmaher)
Attachment #8858167 - Flags: review?(erahm)
Comment on attachment 8858167 [details] [diff] [review]
mach-awsy-windows-v2.patch

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

thanks bc
Attachment #8858167 - Flags: review?(jmaher) → review+
Attachment #8858167 - Flags: review?(erahm) → review+
Pushed by bclary@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e27cbd5c699
Support mach awsy-test on Windows, r=erahm, jmaher.
https://hg.mozilla.org/mozilla-central/rev/7e27cbd5c699
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [checkin-needed-beta]
Component: General → AWSY
You need to log in before you can comment on or make changes to this bug.