-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
CircleCI: Run flake8 tests on both Python 2 and Python 3 #8878
Conversation
The flake8 tests on Python 3 are _temporarily_ run with __--exit-zero__ so we can see our porting progress without failing the builds.
Thanks @cclauss! Good idea. I think the old Also please add yourself to AUTHORS. |
Btw, it may be best to run the python2 and 3 tests in a single circleCI job - as it's fast enough, and avoids any constant job overhead. |
If we keep them separate, it will be easier to drop Python 2 in ~184 days. |
Where exactly are the required vs. optional tests stored? I.E. In which file does the old |
I added myself to authors in #8876 |
I'm a big fan of the python3 upgrade, and I even opened the bug for that: #7198. However, I don't think that the ending of upstream support for python2 is going to be the forcing factor here. Emscripten has a complex set of objectives and a diverse set of users. For example we have used on ancient version of OSX who have a system python that is years old already, so we already support customers on unsupported pythons. So yes, by all means lets work towards removing python2, but I don't see that specific date as the main driver here. |
I agree with @kripken.. lets just add it to the existing flake8 job. I think it will actually make removing it easier... or at least the patch will be smaller to remove it. |
I think those test errors were temporary breakage from earlier, please merge in latest incoming to here. Otherwise this looks good to me. I assume the |
To fix the tests, I think we need to merge |
Btw, does anyone know if there a way for project maintainers to do such a merge themselves? I've seen other projects have an option "let other people add commits" but I'm not sure how to enable that in this repo. |
Oh, it sounds like the PR creator can allow maintainers to add commits, but no one else, https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork So that might not help us much it still requires a manual step for PR submitters for us to help merge incoming in. But whatever people prefer to do is fine with me. |
144 days until Python 2 end of life.
The E999 syntax errors must be fixed to be Python 3 compatible. #9200 and #9201 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm but @sbc100 knows the most about this.
.circleci/config.yml
Outdated
@@ -242,9 +242,12 @@ jobs: | |||
name: install pip | |||
command: | | |||
apt-get update -q | |||
apt-get install -q -y python-pip | |||
apt-get install -q -y python-pip python3-pip | |||
- run: pip install flake8==3.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably upgrade this version too.. (otherwise we could end up with conflicting warnings). Maybe either upstate it as part of this CL or open a separate one if you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like #8877 was created for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8877 only ran flake8 on legacy Python.
The flake8 tests on Python 3 are temporarily run with --exit-zero so we can see our porting progress without failing the builds.
Thank you for submitting a pull request!
If this is your first PR, make sure to add yourself to AUTHORS.