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

update python version in build for html/canvas #24685

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

yiyix
Copy link
Contributor

@yiyix yiyix commented Jul 21, 2020

It's currently building with virtualenv -p python2 .virtualenv, which requires python2 specifically. Updated to use python, so it's compatible with both python2 and python3.

@yiyix
Copy link
Contributor Author

yiyix commented Jul 21, 2020

The concern is initially raised here:
6a53164

@Hexcles Hexcles changed the title update python versionin build for html/canvas update python version in build for html/canvas Aug 12, 2020
@stephenmcgruer
Copy link
Contributor

Taking notes for my own understanding:

  • This is run by update-built-tests.sh, which is run by ./tools/ci/ci_built_diff.sh
  • ./tools/ci/ci_built_diff.sh is run during the CI, in the update-built Taskcluster step
  • It triggers for pull requests, if they match the update_built glob, which expands to:
    "update_built": ["update-built-tests\\.sh",
                     "infrastructure/",
                     "html/",
                     "mimesniff/",
                     "css/css-ui/",
                     "WebIDL"],

What I'm a little confused by is the conversation in 6a53164 - it seems to imply that one set of people had problems running this in Python 2 (mysteryDate mentioned this) whilst another set had problems running this in Python 3 (yiyix said this).

The goal of this change seems to be to let it use whatever the system Python is, but it seems like we aren't sure that will always work?

Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking to Yi, they believe that the script works with both python 2 and python 3, so it seems like this is the correct change to me. It certainly passes the CI in python 2 at least.

I would like to give @jgraham and @Hexcles 24 hours to object, then I will approve + merge this after that.

@mysteryDate
Copy link
Contributor

@jgraham Do you object to this, you thought the CI bots were mad when we made it version agnostic?

@Hexcles
Copy link
Member

Hexcles commented Aug 13, 2020

FWIW, I don't object to this if it works in both 2 and 3, but I remember this was pinned to Python 2 specifically because of some issues on (Gecko?) CI.

@yiyix
Copy link
Contributor Author

yiyix commented Aug 13, 2020

In my local set up, it works for both python 2 and 3. My teammate @mysteryDate did some refactor here and he is able to re-generate all script tests with both python2 and python3.

@stephenmcgruer
Copy link
Contributor

It could be because Mozilla use an older Python 3 internally (I believe they were still targeting Python 3.5 last time I checked). Let me ping @jgraham again and see if they know - sorry @yiyix to drag this on further :(

That said, I did try locally with 3.5.6 and the build.sh script worked fine for me in that environment.

@jgraham
Copy link
Contributor

jgraham commented Aug 13, 2020

I don't have any specific concerns about Gecko CI here; I don't think we run this. I'm somewhat wary of getting different results from Python 2 vs 3 in the canvas stuff, but I suppose I'm also pretty wary of getting different results based on a number of other factors there and it seems to work out.

@stephenmcgruer stephenmcgruer merged commit 1680a1d into web-platform-tests:master Aug 13, 2020
@yiyix
Copy link
Contributor Author

yiyix commented Aug 13, 2020

@jgraham: in the cl that i mentioned above, we tried to generate all canvas wpt tests with python2 and python3 and they give exactly the same result. Hopefully, this can give you more confidence in this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants