oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.72k stars 3.85k forks source link

Subprocess.call conversion to subprocess.check_call #7647

Closed kevinlee12 closed 4 years ago

kevinlee12 commented 4 years ago

In some parts of our more recently migrated scripts, we have call subprocess.call, which doesn't surface errors. We should change those calls to subprocess.check_call (see this doc) so that those errors surface immediately.

kevinlee12 commented 4 years ago

@lilithxxx, just an fyi to prioritize completing the bash script conversion before moving onto this ticket.

seanlip commented 4 years ago

As an FYI I think this also happens for the lint checks -- I remember seeing cases where one of the js/ts linters has errors (or can't even run, because of missing dependencies, thus throwing an exception) but the top-level linter mistakenly claimed "success" anyway during the pre-push checks. Unfortunately, I forgot to preserve the log, but just wanted to point out that this might need looking into, too.

seanlip commented 4 years ago

Oh ... I did preserve the log! Here it is. It arose when I was trying to figure out the configparser issue that I fixed in #7638.

As you can see, there's an entire exception in the log ... and yet the whole thing is still running to completion, claiming "success" all the way. This seems like a rather serious issue (the push would have succeeded had I not manually cancelled it).

Modified files in testbranch:
[FileDiff(status='M', name='extensions/interactions/base-validator.spec.ts'),
 FileDiff(status='M', name='scripts/install_third_party_libs.py')]

Files to lint in testbranch:
['extensions/interactions/base-validator.spec.ts',
 'scripts/install_third_party_libs.py']

Starting Js, Ts, Python, HTML, and CSS linter...
Total js and ts files:  1
Linting JS and TS files.
There are no CSS files to lint.
There are no CSS files to lint.
Linting 1 Python files
Linting 1 Python files for Python 3 compatibility.
Using config file /opensource/oppia/.pylintrc
Using config file /opensource/oppia/.pylintrc
Process Process-9:
Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 267, in _bootstrap
    self.run()
  File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
Process Process-8:
    self._target(*self._args, **self._kwargs)
  File "/opensource/oppia/scripts/pre_commit_linter.py", line 1618, in _lint_py_files_for_python3_compatibility
Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 267, in _bootstrap
    self.run()
  File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "/opensource/oppia/scripts/pre_commit_linter.py", line 1548, in _lint_py_files
    current_files_to_lint + ['--py3k'], exit=False).linter
  File "/opensource/oppia_tools/pylint-1.9.4/pylint/lint.py", line 1318, in __init__
    exit=False).linter
  File "/opensource/oppia_tools/pylint-1.9.4/pylint/lint.py", line 1318, in __init__
    linter.load_plugin_modules(plugins)
  File "/opensource/oppia_tools/pylint-1.9.4/pylint/lint.py", line 500, in load_plugin_modules
    module = modutils.load_module_from_name(modname)
  File "/opensource/oppia_tools/pylint-1.9.4/astroid/modutils.py", line 190, in load_module_from_name
    return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
  File "/opensource/oppia_tools/pylint-1.9.4/astroid/modutils.py", line 232, in load_module_from_modpath
    mp_file, mp_filename, mp_desc = imp.find_module(part, path)
ImportError: No module named pylint_quotes
    linter.load_plugin_modules(plugins)
  File "/opensource/oppia_tools/pylint-1.9.4/pylint/lint.py", line 500, in load_plugin_modules
    module = modutils.load_module_from_name(modname)
  File "/opensource/oppia_tools/pylint-1.9.4/astroid/modutils.py", line 190, in load_module_from_name
    return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
  File "/opensource/oppia_tools/pylint-1.9.4/astroid/modutils.py", line 232, in load_module_from_modpath
    mp_file, mp_filename, mp_desc = imp.find_module(part, path)
ImportError: No module named pylint_quotes
Js and Ts linting finished.
Validating and parsing JS and TS files ...

SUCCESS  Sorted dependencies check passed

Linting HTML files.

Summary of Errors:
----------------------------------------
SUCCESS  CODEOWNERS file check passed

SUCCESS  HTML tag and attribute check passed

SUCCESS   HTML linting passed
HTML linting finished.

SUCCESS   1 JavaScript and Typescript files linted (0.7 secs)

(1 files checked, 0 errors found)
SUCCESS Pattern checks passed
SUCCESS  Extra JS files check passed

SUCCESS  JS and TS Component name and count check passed

SUCCESS  Directive scope check passed

SUCCESS  Controller dependency line break check passed

(1 files checked, 0 errors found)
SUCCESS Pattern checks passed

SUCCESS Division operator check passed

SUCCESS   Import order checks passed

SUCCESS   Comments check passed

SUCCESS   Docstring check passed

---------------------------
All Checks Passed.
---------------------------
Checking if node.js is installed in /opensource/oppia/../oppia_tools
Checking if yarn is installed in /opensource/oppia/../oppia_tools
Environment setup completed.
Checking whether Google App Engine is installed in /opensource/oppia/../oppia_tools/google_appengine_1.9.67/google_appengine
Checking whether google-cloud-sdk is installed in /opensource/oppia/../oppia_tools/google-cloud-sdk-251.0.0/google-cloud-sdk
Checking if node.js is installed in /opensource/oppia/../oppia_tools
Checking if yarn is installed in /opensource/oppia/../oppia_tools
Environment setup completed.
Checking whether Google App Engine is installed in /opensource/oppia/../oppia_tools/google_appengine_1.9.67/google_appengine
Checking whether google-cloud-sdk is installed in /opensource/oppia/../oppia_tools/google-cloud-sdk-251.0.0/google-cloud-sdk
Checking if pylint is installed in /opensource/oppia/../oppia_tools
Checking if Pillow is installed in /opensource/oppia/../oppia_tools
Checking if pylint-quotes is installed in /opensource/oppia/../oppia_tools
Installing pylint-quotes
Checking if pip is installed on the local machine
pylint-quotes 0.2.1
Collecting pylint-quotes==0.2.1
  Could not find a version that satisfies the requirement pylint-quotes==0.2.1 (from versions: 0.1.3, 0.1.5, 0.1.6, 0.1.7, 0.1.8)
No matching distribution found for pylint-quotes==0.2.1
Checking if webtest is installed in /opensource/oppia/../oppia_tools
Checking if isort is installed in /opensource/oppia/../oppia_tools
Checking if pycodestyle is installed in /opensource/oppia/../oppia_tools
Checking if esprima is installed in /opensource/oppia/../oppia_tools
Checking if browsermob-proxy is installed in /opensource/oppia/../oppia_tools
Checking if selenium is installed in /opensource/oppia/../oppia_tools
Checking if PyGithub is installed in /opensource/oppia/../oppia_tools
Installing third-party JS libraries and zip files.
yarn install v1.17.3
[1/4] Resolving packages...
success Already up-to-date.
Done in 0.51s.
Checking whether Skulpt is installed in third_party
Installing pre-commit hook for git
Symlink already exists
Making pre-commit hook file executable ...
seanlip commented 4 years ago

@kevinlee12 -- reassigning this to you, since @lilithxxx isn't around. Could you take it up please?

kevinlee12 commented 4 years ago

sure!