Skip to content

Commit

Permalink
Make canvas tests use python2 for build
Browse files Browse the repository at this point in the history
  • Loading branch information
@jgraham
jgraham committed Jul 7, 2020
1 parent 76a0eee commit 6a53164
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion html/canvas/tools/build.sh
View file
Expand Up @@ -2,6 +2,6 @@
set -ex

cd "${0%/*}"
virtualenv -p python .virtualenv
virtualenv -p python2 .virtualenv
.virtualenv/bin/pip install pyyaml cairocffi
.virtualenv/bin/python gentest.py

6 comments on commit 6a53164

@mysteryDate
Copy link
Contributor

@mysteryDate mysteryDate commented on 6a53164 Jul 17, 2020 •
edited

Choose a reason for hiding this comment

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

Why is this necessary? I've been running it in python3 without issue.

@mysteryDate
Copy link
Contributor

Choose a reason for hiding this comment

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

#23357

@jgraham
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume that it failed CI without this change, but I don't remember the details.

In any case we shouldn't say python and expect py3 to be used, and we should be specific and consistent about which version we're using for this.

@mysteryDate
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... it was passing the CI earlier. Do you have the failure on-hand? I agree about being explicit, but IMO we should explicitly use python3. My team and I have had issues running this locally with python2 versions of virtualenv and cairo.
@yiyix: You were unable to build with python2, correct?

@jgraham
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have the failure; it's hard to find when commits are rewritten :/

I don't really mind this requiring Python 3 since it isn't run that often, but in general the project currently requires Python 2 and some parts have experimental support for Python 3. So it would be a substantive change to make, and probably deserves wider review (probably qualifies for an RFC).

@yiyix
Copy link
Contributor

@yiyix yiyix commented on 6a53164 Jul 20, 2020

Choose a reason for hiding this comment

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

I was unable to build it with python 3.
This line virtualenv -p python .virtualenv allows the system to use the which ever python version available in /usr/local/bin/python. So the build file works with both python2 and python 3.

I agree that this change needs a wider review, I will submit a separate review request.

Please sign in to comment.