Skip to content

Conversation

@cclauss
Copy link
Contributor

@cclauss cclauss commented Jul 24, 2018

An alternative approach to #21952 that runs flake8 in parallel Travis CI jobs on both Python 2.7 and Python 3.7. The Python 3.7 job is run in allow_failures mode.

See #21952 for more details on the --exclude and --select choices. A mix of these two PRs is also possible.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax
Copy link
Member

My gut feeling says that I’d prefer #21952 (but it’s nice to have two options laid out!).

/cc @nodejs/build-files @nodejs/python

@cclauss
Copy link
Contributor Author

cclauss commented Jul 24, 2018

I tend to like this one because the committer gets a "fast fail".

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

AFAICT this change is unneeded if #21952 lands.

- make -j2 V=
script:
- make -j2 test-ci
- os: linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be covered by the lint-ci job

- flake8 --version
- EXCLUDE=./deps/npm/node_modules/node-gyp,./deps/v8,./src/noperfctr_macros.py,./src/notrace_macros.py,./tools
- flake8 . --count --exclude=${EXCLUDE} --select=E901,E999,F821,F822,F823 --show-source --statistics
- os: linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed ATM. There are many script that are incompatible anyway.

- export CXX="ccache clang++ -Qunused-arguments"
- export CC="ccache clang -Qunused-arguments -Wno-unknown-warning-option"
- export JOBS=2
- export JOBS=4
Copy link
Contributor

Choose a reason for hiding this comment

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

Should check why this was 2 before changing

@refack
Copy link
Contributor

refack commented Jul 24, 2018

@cclauss thanks again.
I'd much rather have all the linting logic collected in one place, and keep .travis.yml minimal, as we run that code on our own CI cluster as well.

@refack refack added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. python PRs and issues that require attention from people who are familiar with Python. labels Jul 24, 2018
@cclauss
Copy link
Contributor Author

cclauss commented Jul 24, 2018

Is there a way to notify the committer that their PR contains errors detected by https://ci.nodejs.org? My sense is that my PR could contain errors but Travis CI and GitHub would be reporting all green.

@cclauss cclauss closed this Jul 24, 2018
@cclauss cclauss deleted the run-flake8-from-travis branch July 24, 2018 18:16
@richardlau
Copy link
Member

@cclauss The Node.js CI jobs should be posting status back to the associated PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants