Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This adds the ability to download latest Servo for `wpt run` #9687

Merged
merged 1 commit into from Mar 12, 2018

Conversation

Projects
None yet
5 participants
@kriti21
Copy link
Contributor

kriti21 commented Feb 27, 2018

Closes #9544


This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Feb 27, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Feb 27, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 27, 2018

Build PASSED

Started: 2018-03-12 19:46:00
Finished: 2018-03-12 20:02:35

View more information about this build on:

else:
extension = ".dmg"

return "%s" % (extension)

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 27, 2018

Contributor

This is the same as return extension.

This comment has been minimized.

Copy link
@jdm

jdm Mar 9, 2018

Contributor

This is still the case.

extension = ".msi"
else:
bits = ""
extension = ".dmg"

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 27, 2018

Contributor

All of this is unused; I'd suggest returning a tuple (return (platform, extension)), though, so you don't need two methods.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Mar 8, 2018

Contributor

Did you test the new code? I don't see how it can work, because you return a single string instead of a tuple.

This comment has been minimized.

Copy link
@kriti21

kriti21 Mar 8, 2018

Author Contributor

I am having trouble in understanding how to test my changes locally. Could you please guide me on that? @Ms2ger

This comment has been minimized.

Copy link
@jdm

jdm Mar 9, 2018

Contributor

There is still a bunch of unused code in this method. I agree that combining the platform and extension methods together makes sense.

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from decd12c to 83fd42b Mar 7, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 7, 2018

Sorry for the delay. I have made the suggested changes. Please review :) @Ms2ger

@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Mar 8, 2018

I don't know about testing. @jdm or @jgraham might.

@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 8, 2018

The original issue suggested a command to run that is expected to work, I believe.

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 8, 2018

Here is screenshot of that command. Please help me with these errors. @Ms2ger @jdm @gsnedders

testm


return "%s%s" % (platform, extension)

def install_browser_engine(self, dest=None):

This comment has been minimized.

Copy link
@jdm

jdm Mar 8, 2018

Contributor

You renamed the install method to install_browser_engine, so the error message is letting you know that it expects the install method.

This comment has been minimized.

Copy link
@kriti21

kriti21 Mar 8, 2018

Author Contributor

Thanks I have corrected this. This is what it shows now. @jdm

testm1

This comment has been minimized.

Copy link
@jdm

jdm Mar 8, 2018

Contributor

That looks like you restored the original install method to its original implementation, which just raises a NotImplementedError exception.

This comment has been minimized.

Copy link
@jdm

jdm Mar 8, 2018

Contributor

Also, rather than attaching screenshots, I recommend copying the text out of your console and pasting it between blocks of characters like this: https://help.github.com/articles/creating-and-highlighting-code-blocks/

This comment has been minimized.

Copy link
@kriti21

kriti21 Mar 8, 2018

Author Contributor

@jdm I. Isn't this the implementation of install method or I have done something wrong here? Why is it giving NotImplementedError after renaming it to install ?

This comment has been minimized.

Copy link
@jdm

jdm Mar 8, 2018

Contributor

@kriti21 It's hard to say without looking at the actual code you're running. Please push your changes if you'd like me to comment on them :)

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 83fd42b to 7e82c9c Mar 8, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 8, 2018

@jdm pushed the changes :)

@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 8, 2018

@kriti21 What is the output from the command?

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 8, 2018

./wpt run --install-browser servo dom/historical.html command gives this output. @jdm

 0:00.11 INFO Installing browser
Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/wpt.py", line 132, in main
    rv = script(*args, **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 404, in run
    **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 382, in setup_wptrunner
    kwargs["binary"] = setup_cls.install(venv)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 335, in install
    raise NotImplementedError
NotImplementedError
@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 8, 2018

Oh, I see. The code is complaining about the class in run.py, but all your changes are in browser.py. You should be able to remove the install method in run.py, because it will fall back to this one instead.

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 7e82c9c to cf8284b Mar 8, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 8, 2018

@jdm i have pushed with changes again. This is what it shows now. Please help correct this as well :)

 0:00.00 INFO Installing browser
Download and install servo [Y/n]? Y
Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/wpt.py", line 132, in main
    rv = script(*args, **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 405, in run
    **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 383, in setup_wptrunner
    kwargs["binary"] = setup_cls.install(venv)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 336, in install
    return self.browser.install(venv.path)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/browser.py", line 474, in install
    unzip(get(url).raw, dest)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/utils.py", line 88, in unzip
    with zipfile.ZipFile(fileobj) as zip_data:
  File "/usr/lib/python2.7/zipfile.py", line 770, in __init__
    self._RealGetContents()
  File "/usr/lib/python2.7/zipfile.py", line 811, in _RealGetContents
    raise BadZipfile, "File is not a zip file"
zipfile.BadZipfile: File is not a zip file
@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 8, 2018

It looks like it's expecting a zip file, but on linux we provide a tar.gz archive instead. I recommend using the untar function instead, and raising an exception if we try to use this on Windows or macOS, because those require even more specialized installation procedures (since they are not zips or tars).

@kriti21 kriti21 force-pushed the kriti21:kbranch branch 2 times, most recently from 12ff2fe to 832b3ec Mar 9, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 9, 2018

@jdm This is the error that occurs now.

 0:00.00 INFO Installing browser
Download and install servo [Y/n]? Y
Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/wpt.py", line 132, in main
    rv = script(*args, **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 405, in run
    **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 383, in setup_wptrunner
    kwargs["binary"] = setup_cls.install(venv)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/run.py", line 336, in install
    return self.browser.install(venv.path)
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/browser.py", line 480, in install
    st = os.stat(path)
TypeError: coercing to Unicode: need string or buffer, NoneType found

I know this error occurs when path specified is None. But the same approach seems to work for other browsers. Please help.

@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 9, 2018

path is the result of calling find_executable, so I recommend tracing the code there to figure out what's going wrong.

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 9, 2018

@jdm Could you tell me what should be the output of find_executable. On the website it says to download the tar file and run ./servo inside servo directory (for linux). How can I do this in this code ?

@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 9, 2018

@kriti21 I think we probably want to follow the example of the Firefox installation code, which looks quite similar to what we're trying to do for Servo: https://github.com/kriti21/web-platform-tests/blob/832b3ec2294cf45d856b31c6479183eb98de9788/tools/wpt/browser.py#L132-L133

The second argument to find_executable allows us to specify the precise directory in which we expect to find the servo binary: https://github.com/python/cpython/blob/master/Lib/distutils/spawn.py#L169-L174

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 832b3ec to 8cfd02b Mar 9, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 9, 2018

@jdm With the code that i have pushed now, the command ./wpt run --install-browser servo dom/historical.html halts after displaying this. Nothing happens even after waiting for half an hour or so.

0:00.06 INFO Installing browser
Download and install servo [Y/n]?
@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 9, 2018

@kriti21 I recommend tracing the code that runs and adding print statements to figure out what it's doing. You can start at https://github.com/kriti21/web-platform-tests/blob/8cfd02b23663ac97b4db0648e3656ca7b46c75e5/tools/wpt/run.py#L381.

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 8cfd02b to 9635a7d Mar 9, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 9, 2018

Here is the output i get locally. Please review @jdm

 0:00.10 INFO Installing browser
Download and install servo [Y/n]? Y
 2:35.39 INFO Updating test manifest /home/kriti/mozdev/web-platform-tests/MANIFEST.json
 2:35.39 INFO STDOUT: INFO:manifest:Skipping manifest download because existing file is recent
 2:38.87 INFO STDOUT: INFO:manifest:Updating manifest
 4:21.64 INFO STDOUT: DEBUG:manifest:Opening manifest at /home/kriti/mozdev/web-platform-tests/MANIFEST.json
 4:27.22 INFO Using 1 client processes
 4:27.37 INFO Starting http server on web-platform.test:8000
 4:27.38 INFO Starting http server on web-platform.test:8001
 4:27.38 SUITE_START: web-platform-test - running 1 tests
 4:27.38 INFO Running reftest tests
 4:27.39 INFO No reftest tests to run
 4:27.39 INFO Running wdspec tests
 4:27.39 INFO No wdspec tests to run
 4:27.39 INFO Running testharness tests
 4:27.39 INFO Starting runner
 4:27.40 INFO Starting http server on web-platform.test:8443
 4:27.42 TEST_START: /dom/historical.html
 4:48.28 INFO Pausing until the browser exits
 4:48.28 TEST_END: Test OK. Subtests passed 72/72. Unexpected 0
 4:48.28 INFO Pausing until the browser exits
 4:48.28 INFO No more tests
 4:48.30 WARNING u'runner_teardown': ()
 4:48.30 INFO Got 0 unexpected results
 4:48.30 SUITE_END

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 73 checks (1 tests, 72 subtests)
Expected results: 73
OK
 4:48.39 INFO Closing logging queue
 4:48.39 INFO queue closed
@jdm

This comment has been minimized.

Copy link
Contributor

jdm commented Mar 9, 2018

That looks like it's behaving as expected?

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 9, 2018

Yes. It opened a window for servo just like it does for mozilla. @jdm

extension = ".msi"
else:
bits = ""
extension = ".dmg"

This comment has been minimized.

Copy link
@jdm

jdm Mar 9, 2018

Contributor

There is still a bunch of unused code in this method. I agree that combining the platform and extension methods together makes sense.

else:
extension = ".dmg"

return "%s" % (extension)

This comment has been minimized.

Copy link
@jdm

jdm Mar 9, 2018

Contributor

This is still the case.

extension = self.find_extension(self.platform_string())
url = "https://download.servo.org/nightly/%s/servo-latest%s" % (self.platform_string(), extension)

if format == "zip":

This comment has been minimized.

Copy link
@jdm

jdm Mar 9, 2018

Contributor

Where does this come from?

This comment has been minimized.

Copy link
@kriti21

kriti21 Mar 9, 2018

Author Contributor

If I use the same method to return tuple of platform and extension, i cannot use them separately in the generating url which is needed here.

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 9635a7d to 21ad7a9 Mar 9, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 9, 2018

@jdm I have removed the useless code. However, if I use the same function to return a tuple of platform and extension, I don't think i can use them at separate places in generating URL which is needed here.

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 21ad7a9 to 4f6a6dd Mar 10, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 10, 2018

Also as much as I understand the travis build fail logs, it says line 481 in browser.py has extra whitespace. I am not sure if its correct.

dest = os.pwd

extension = self.find_extension(self.platform_string())
url = "https://download.servo.org/nightly/%s/servo-latest%s" % (self.platform_string(), extension)

This comment has been minimized.

Copy link
@jdm

jdm Mar 12, 2018

Contributor

Rather than two separate functions, if there is a single function that returns (platform, extension), we can do:

def platform_components()
    ...
    return (platform, extension)

url = "https://download.servo.org/nightly/%s/servo-latest%s" % self.platform_components()
extension = self.find_extension(self.platform_string())
url = "https://download.servo.org/nightly/%s/servo-latest%s" % (self.platform_string(), extension)

if self.platform_string() == "linux":

This comment has been minimized.

Copy link
@jdm

jdm Mar 12, 2018

Contributor

Since we only support linux right now, we can remove any code that checks for the platform or tries to support other platforms.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Mar 12, 2018

Contributor

And if we keep this, maybe platform_components or whatever should return the unzip/untar function as well, to localize the dependency on the kind of files that are used on each platform.

This comment has been minimized.

Copy link
@kriti21

kriti21 Mar 12, 2018

Author Contributor

I am not sure about returning unzip / untar function from platform_components since platform and extension values are used to form a url which downloads the required file that is extracted using untar / unzip function. Would not including url or filename in platform_components make the function more complicated? @jdm @Ms2ger

This comment has been minimized.

Copy link
@jdm

jdm Mar 12, 2018

Contributor

@kriti21 The previous comment meant something like this:

def platform_components(self):
    if ...:
        return ("linux", ".tar.gz", untar)

platform, extension, decompress = self.platform_components()
url = "..." % (platform, extension)
decompress(get(url).raw, dest=dest)

Does that make sense?

This comment has been minimized.

Copy link
@kriti21

kriti21 Mar 12, 2018

Author Contributor

Oh okay! Thanks :)

untar(get(url).raw, dest=dest)
else:
unzip(get(url).raw, dest=dest)

This comment has been minimized.

Copy link
@jdm

jdm Mar 12, 2018

Contributor

The TravisCI build is complaining that this blank line contains trailing whitespace.

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 4f6a6dd to 2883d22 Mar 12, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 12, 2018

@jdm Please review my changes. Also I haven't removed the options for platforms other than linux yet. Tell me if I should do that in this code.

@jdm
Copy link
Contributor

jdm left a comment

With this change it should be good to merge. Thanks for sticking with this task!

decompress = unzip
else:
extension = ".dmg"
decompress = unzip

This comment has been minimized.

Copy link
@jdm

jdm Mar 12, 2018

Contributor

Given that unzip will not work for MSI or DMG files, we should do this instead:

elif platform == "win" or platform == "mac":
    raise ValueError("Unable to extract Servo package for current platform")

@kriti21 kriti21 force-pushed the kriti21:kbranch branch from 2883d22 to f497498 Mar 12, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

kriti21 commented Mar 12, 2018

@jdm I have made the final change. Hope this is ready to merge now. Thanks a lot. :)

@jdm

jdm approved these changes Mar 12, 2018

Addressed.

@jdm jdm merged commit 2f110a4 into web-platform-tests:master Mar 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.