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

Deprecate --quiet recursive option #6156

Merged
merged 25 commits into from Aug 23, 2018

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 17, 2018

Problem

Fixes #6142.

Solution

  • Add flag_matchers memoized property on Options to wrap predicates on CLI options which may raise a deprecation warning.
  • Convert deprecated scope checking to use flag_matchers in the _check_deprecated_scope(self) method in options.py.
  • Make tests/python/pants_test/option:integration include all *_integration.py (it wasn't running test_quiet_option_integration.py before -- it looks like that test still passes now, however).
  • Add _match_recursive_quiet_flag(self) and add to flag_matchers.

Result

  • -q or --quiet continues to silence output when used as a global option, but a deprecation warning is printed to stderr if it is used recursively.
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 17, 2018

@cosmicexplorer : This isn't the first time we've had that issue with deprecation. It might call for a variant of deprecation (maybe as an argument to the deprecation methods) that causes it to use stderr directly, rather than the logger?

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 18, 2018

That seems really doable, because we are doing this in the body of the Options class, so we can make sure we always respect a global -q by not logging to stderr if that global option is set, but if not, we would be able to know that it is safe to log directly to stderr. Does logging typically go out into anything else but the standard streams (i.e. would we probably want to use the logger as well as go directly to stderr)? Or can we assume that if a global -q is on that we don't need to log anything anywhere?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 18, 2018

That seems really doable, because we are doing this in the body of the Options class, so we can make sure we always respect a global -q by not logging to stderr if that global option is set

Hm, I wasn't suggesting having deprecations inspect the -q flag, per-se... just suggesting an explicit option to the deprecation methods... ie:

@deprecated(to_stderr=True)

... for example.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 24, 2018

Oh, that's what I meant, I was responding to the next step in my head, which is why that decorator would be safe to use to deprecate -q (as in, why we would know that going to stderr would be the right move).

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:deprecate-quiet-recursive-option branch from 8c48213 to bd1030b Jul 24, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Jul 24, 2018

Added ensure_stderr and frame_info arguments to warn_or_error and deprecated, and used it to warn to stderr directly. Added the beginnings of testing for the warning, but failing currently because I need to figure out how to use the stdio_as_tempfiles() method I extracted from the contextutil testing so we can record the warning to stderr in a contextmanager. 647ec21 could probably be reverted if testing for whether the argument is successfully deprecated is not deemed necessary (maybe not, but it might also be good to test ensure_stderr?).

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:deprecate-quiet-recursive-option branch from 647ec21 to b4e2284 Aug 11, 2018

@cosmicexplorer cosmicexplorer changed the title [WIP] Deprecate --quiet recursive option through new DeprecatedFlagMatcher type Deprecate --quiet recursive option through new DeprecatedFlagMatcher type Aug 11, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 11, 2018

This is now ready for review. I decided to avoid a ton of complexity by just adding integration tests instead of trying to capture stderr within a unit test, but that results in two extra pants invocations each time we run CI, which is eh.

@stuhood
Copy link
Member

stuhood left a comment

Thanks! It's possible that the abstraction around DeprecatedMatchers is overkill, but up to you.

from pants.util.objects import datatype


class CallableWrapper(datatype(['callable_object'])):

This comment has been minimized.

@stuhood

stuhood Aug 12, 2018

Member

I don't think that this class is necessary... a "callable wrapper" is easiest expressed with just an anonymous function?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

I don't think so either, I was able to remove it in ~10 seconds so it obviously wasn't doing anything special. See fb95e63!

return super(CallableWrapper, cls).__new__(cls, callable_object)


class DeprecatedFlagMatcher(datatype([('scope_flags_fun', CallableWrapper)])):

This comment has been minimized.

@stuhood

stuhood Aug 12, 2018

Member

If you think this class is worth keeping (vs maybe inlining as static/class methods?), please add docstrings for it. It's good to think about will still be useful after the deprecation.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Removed, see fb95e63!


_quiet_flag_regex = re.compile(r'\A(\-q|\-\-([^=]+\-)?quiet(=.*)?)\Z')

def _match_recursive_quiet_flag(self, scope, flags):

This comment has been minimized.

@stuhood

stuhood Aug 12, 2018

Member

Would be good to note here that the entire method can be removed after the deprecation.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Also added in fb95e63!

@@ -47,7 +49,37 @@ def subsystem(scope):
return ScopeInfo(scope, ScopeInfo.SUBSYSTEM)


class OptionsTest(unittest.TestCase):
class OptionsTestBase(unittest.TestCase):

This comment has been minimized.

@stuhood

stuhood Aug 12, 2018

Member

It looks like this extraction wasn't used... is a file missing here?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

This is an artifact of an earlier implementation which had a really screwy way to test stderr. This will be reverted.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

This was reverted as of 338a11c!

removal_version='1.13.0.dev.0',
deprecated_entity_description="Using the -q or --quiet option recursively "
"(after a goal name on the command line)",
hint="The -q or --quiet flags can be specified globally by providing them on the "

This comment has been minimized.

@stuhood

stuhood Aug 12, 2018

Member

s/can/should/

This comment has been minimized.

@cosmicexplorer

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:deprecate-quiet-recursive-option branch from eef7bb7 to fb95e63 Aug 18, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 18, 2018

I have responded to all comments and updated the OP. I am experiencing some breakage locally (on arch linux) on the commit hook script that is reproing even after running rm -rf ~/.cache/pants/python_cache/requirements/CPython-2.7.15:

> ./build-support/bin/pre-commit.sh
* Checking packages
* Checking headers
* Checking for banned imports
* Checking formatting of rust files
Ensuring generated code is present for downstream formatting checks...
    Finished dev [unoptimized + debuginfo] target(s) in 0.19s
* Checking for bad shell patterns
* Checking imports

* Checking lint
**** Failed to install s3-log-parse-0.1.1 (caused by: NonZeroExit("received exit code 1 during execution of `[u'/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/bin/python2.7', '-', 'bdist_wheel', '--dist-dir=/tmp/tmp4bf64X']` while trying to execute `[u'/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/bin/python2.7', '-', 'bdist_wheel', '--dist-dir=/tmp/tmp4bf64X']`",)
):
stdout:

Installed /tmp/easy_install-x4oIWI/flake8-3.5.0/.eggs/pytest_runner-4.2-py2.7.egg

Installed /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs/flake8-3.5.0-py2.7.egg
Searching for nose>=1.0
Reading https://pypi.python.org/simple/nose/
Downloading https://files.pythonhosted.org/packages/58/a5/0dc93c3ec33f4e281849523a5a913fa1eea9a3068acfa754d44d88107a44/nose-1.3.7.tar.gz#sha256=f1bffef9cbc82628f6e7d7b40d7e255aefaa1adb6a1b1d26c69a8b79e6208a98
Best match: nose 1.3.7
Processing nose-1.3.7.tar.gz
Writing /tmp/easy_install-T10XJ4/nose-1.3.7/setup.cfg
Running nose-1.3.7/setup.py -q bdist_egg --dist-dir /tmp/easy_install-T10XJ4/nose-1.3.7/egg-dist-tmp-Hw6dzY
creating /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs/nose-1.3.7-py2.7.egg
Extracting nose-1.3.7-py2.7.egg to /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs

Installed /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs/nose-1.3.7-py2.7.egg
Searching for mccabe<0.7.0,>=0.6.0
Reading https://pypi.python.org/simple/mccabe/
Downloading https://files.pythonhosted.org/packages/06/18/fa675aa501e11d6d6ca0ae73a101b2f3571a565e0f7d38e062eec18a91ee/mccabe-0.6.1.tar.gz#sha256=dd8d182285a0fe56bace7f45b5e7d1a6ebcbf524e8f3bd87eb0f125271b8831f
Best match: mccabe 0.6.1
Processing mccabe-0.6.1.tar.gz
Writing /tmp/easy_install-Z7z5NX/mccabe-0.6.1/setup.cfg
Running mccabe-0.6.1/setup.py -q bdist_egg --dist-dir /tmp/easy_install-Z7z5NX/mccabe-0.6.1/egg-dist-tmp-3qdvCK

Installed /tmp/easy_install-Z7z5NX/mccabe-0.6.1/.eggs/pytest_runner-4.2-py2.7.egg
creating /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs/mccabe-0.6.1-py2.7.egg
Extracting mccabe-0.6.1-py2.7.egg to /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs

Installed /tmp/tmpFdmZx_/s3-log-parse-0.1.1/.eggs/mccabe-0.6.1-py2.7.egg

stderr:
zip_safe flag not set; analyzing archive contents...
warning: no previously-included files matching '*.pyc' found anywhere in distribution
no previously-included directories found matching 'docs/build/'
zip_safe flag not set; analyzing archive contents...
no previously-included directories found matching 'doc/.build'
zip_safe flag not set; analyzing archive contents...
Traceback (most recent call last):
  File "<stdin>", line 9, in <module>
  File "setup.py", line 26, in <module>
    test_suite="tests",
  File "/usr/lib64/python2.7/distutils/core.py", line 111, in setup
    _setup_distribution = dist = klass(attrs)
  File "build/bdist.linux-x86_64/egg/setuptools/dist.py", line 317, in __init__
  File "build/bdist.linux-x86_64/egg/setuptools/dist.py", line 372, in fetch_build_eggs
  File "build/bdist.linux-x86_64/egg/pkg_resources/__init__.py", line 846, in resolve
  File "build/bdist.linux-x86_64/egg/pkg_resources/__init__.py", line 1111, in best_match
  File "build/bdist.linux-x86_64/egg/pkg_resources/__init__.py", line 715, in find
pkg_resources.VersionConflict: (pycodestyle 2.4.0 (/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/lib/python2.7/site-packages), Requirement.parse('pycodestyle<2.4.0,>=2.0.0'))



FAILURE
Exception caught: (<class 'pex.resolver.Untranslateable'>)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/pants_loader.py", line 75, in <module>
    main()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/pants_loader.py", line 71, in main
    PantsLoader.run()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/pants_loader.py", line 67, in run
    cls.load_and_execute(entrypoint)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/pants_loader.py", line 60, in load_and_execute
    entrypoint_main()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/pants_exe.py", line 38, in main
    PantsRunner(exiter, start_time=start_time).run()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/pants_runner.py", line 53, in run
    return runner.run()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/local_pants_runner.py", line 161, in run
    self._run()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/local_pants_runner.py", line 225, in _run
    goal_runner_result = self._maybe_run_v1(run_tracker, reporting)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/local_pants_runner.py", line 178, in _maybe_run_v1
    return goal_runner_factory.create().run()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/goal_runner.py", line 204, in run
    return self._run_goals()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/goal_runner.py", line 175, in _run_goals
    result = self._execute_engine()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/bin/goal_runner.py", line 163, in _execute_engine
    result = engine.execute(self._context, self._goals)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/engine/legacy_engine.py", line 26, in execute
    self.attempt(context, goals)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/engine/round_engine.py", line 233, in attempt
    goal_executor.attempt(explain)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/engine/round_engine.py", line 49, in attempt
    task.execute()
  File "/home/cosmicexplorer/tools/pants/src/python/pants/backend/python/tasks/resolve_requirements.py", line 29, in execute
    pex = self.resolve_requirements(interpreter, self.context.targets(has_python_requirements))
  File "/home/cosmicexplorer/tools/pants/src/python/pants/backend/python/tasks/resolve_requirements_task_base.py", line 83, in resolve_requirements
    dump_requirement_libs(builder, interpreter, req_libs, self.context.log, platforms=maybe_platforms)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/backend/python/tasks/pex_build_util.py", line 101, in dump_requirement_libs
    dump_requirements(builder, interpreter, reqs, log, platforms=platforms)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/backend/python/tasks/pex_build_util.py", line 125, in dump_requirements
    distributions = resolve_multi(interpreter, deduped_reqs, platforms, find_links)
  File "/home/cosmicexplorer/tools/pants/src/python/pants/backend/python/tasks/pex_build_util.py", line 169, in resolve_multi
    use_manylinux=python_setup.use_manylinux)
  File "/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/lib/python2.7/site-packages/pex/resolver.py", line 453, in resolve
    return resolver.resolve(resolvables_from_iterable(requirements, builder))
  File "/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/lib/python2.7/site-packages/pex/resolver.py", line 248, in resolve
    dist = self.build(package, resolvable.options)
  File "/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/lib/python2.7/site-packages/pex/resolver.py", line 323, in build
    dist = super(CachingResolver, self).build(package, options)
  File "/home/cosmicexplorer/tools/pants/build-support/pants_dev_deps.venv/lib/python2.7/site-packages/pex/resolver.py", line 206, in build
    raise Untranslateable('Package %s is not translateable by %s' % (package, translator))

Exception message: Package SourcePackage(u'file:///home/cosmicexplorer/.cache/pants/python_cache/requirements/CPython-2.7.15/s3-log-parse-0.1.1.tar.gz') is not translateable by ChainedTranslator(WheelTranslator, EggTranslator, SourceTranslator)

It doesn't seem like pex supports .tar.gz distributions, and although I could probably follow the example of the others and make a translator, why/where are we downloading these as .tar.gzs anyway?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 21, 2018

@cosmicexplorer : If you're experiencing that issue on master, it would be good to open a separate bug/PR for it. cc @benjyw

In the meantime, this has some relevant looking failures in CI (although they might be resolved by a merge/rebase?)

@cosmicexplorer cosmicexplorer changed the title Deprecate --quiet recursive option through new DeprecatedFlagMatcher type Deprecate --quiet recursive option Aug 21, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 21, 2018

I can no longer reproduce the above behavior, and I can easily reproduce the CI failures, so attacking those now.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:deprecate-quiet-recursive-option branch from 63b3052 to 8c3cc64 Aug 21, 2018

cosmicexplorer added some commits Jul 17, 2018

Turn the unit tests into integration tests.
I wasn't able to figure out how to do this in a unit test.

cosmicexplorer added some commits Aug 11, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:deprecate-quiet-recursive-option branch from 623e3c5 to 9a6fb6c Aug 21, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 21, 2018

It looks like there is an actual Py3 failure on the Py3 shard. Chris can refer you to a internal ticket about how to try out Py3 locally.

cosmicexplorer added some commits Aug 22, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 23, 2018

Not sure why this specific fix worked for py2 vs py3, as I have not been able to easily find documentation of what warnings.showwarning accepts across python versions, but this seems to work reliably.

@cosmicexplorer cosmicexplorer merged commit ad22237 into pantsbuild:master Aug 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment