Skip to content
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

Add verbose flag #247

Closed
wants to merge 3 commits into from

Conversation

john-sandall
Copy link

Initial sketch of a solution to #153

These are my changes so far in their entirety (within environment.py::Environment::create_lockfile()):

verbose = True
process = subprocess.Popen(
    self.pin_command,
    stdout=subprocess.PIPE,
    stderr=None if verbose else subprocess.PIPE,
)
if verbose:
    while True:
        output = process.stdout.readline()
        if output == b'' and process.poll() is not None:
            break
        if output:
            print(output.strip())
    # TODO: Decide what to do about this
    # stdout, stderr = process.poll(), None

I haven't add the CLI argument for verbose as I'm not sure of the best way to hook this up; also I'm not sure if this solution is workable. I've tested it, and it works, but there may be drawbacks from doing this that I'm not aware of and I'm pretty sure that using print(output.strip()) here isn't correct (should it be logger.info() or sys.stdout.write() maybe?).

Hopefully it's a starting point though?

It's passing flake8 and I've run pytest:

ERROR    pip-compile-multi:environment.py:218 Package django was resolved to different versions in different environments: 1.10.8 and 3.1.7
PASSED
tests/test_discover.py::test_discover_nested PASSED
tests/test_pipcompilemulti.py::test_fix_compatible_pin PASSED
tests/test_pipcompilemulti.py::test_no_fix_incompatible_pin PASSED
tests/test_pipcompilemulti.py::test_pin_is_ommitted_if_set_to_ignore PASSED
tests/test_pipcompilemulti.py::test_post_releases_are_kept_by_default PASSED
tests/test_pipcompilemulti.py::test_forbid_post_releases PASSED
tests/test_pipcompilemulti.py::test_parse_references[base.in-refs0] PASSED
tests/test_pipcompilemulti.py::test_parse_references[test.in-refs1] PASSED
tests/test_pipcompilemulti.py::test_parse_references[local.in-refs2] PASSED
tests/test_pipcompilemulti.py::test_split_header PASSED
tests/test_pipcompilemulti.py::test_concatenation PASSED
tests/test_pipcompilemulti.py::test_parse_hashes_with_comment PASSED
tests/test_pipcompilemulti.py::test_parse_hashes_without_comment PASSED
tests/test_pipcompilemulti.py::test_serialize_hashes PASSED
tests/test_pipcompilemulti.py::test_reference_cluster PASSED
tests/test_pipcompilemulti.py::test_parse_vcs_dependencies PASSED
tests/test_pipcompilemulti.py::test_merged_packages_raise_for_conflict
-------------------------------------------------------------------------------------------------------------- live log call ---------------------------------------------------------------------------------------------------------------
ERROR    pip-compile-multi:utils.py:79 Package x was resolved to different versions in different environments: 2 and 1
PASSED
tests/test_pipcompilemulti.py::test_fix_pin_detects_version_conflict
-------------------------------------------------------------------------------------------------------------- live log call ---------------------------------------------------------------------------------------------------------------
ERROR    pip-compile-multi:environment.py:218 Package x was resolved to different versions in different environments: 2 and 1
PASSED
tests/test_upgrade_feature.py::test_upgrade_package_disables_upgrade PASSED
tests/test_utils.py::test_recursive_refs PASSED

====================================================================================================== 49 passed in 114.33s (0:01:54) ======================================================================================================

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #247 (d524ee8) into master (319dc89) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #247   +/-   ##
=======================================
  Coverage   91.89%   91.90%           
=======================================
  Files          30       30           
  Lines         851      852    +1     
  Branches      113      113           
=======================================
+ Hits          782      783    +1     
  Misses         49       49           
  Partials       20       20           
Impacted Files Coverage Δ
pipcompilemulti/environment.py 84.24% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 319dc89...d524ee8. Read the comment docs.

@@ -63,11 +63,23 @@ def create_lockfile(self):
if sink_out_path and sink_out_path != self.outfile:
original_in_file = self._read_infile()
self._inject_sink()

verbose = True
process = subprocess.Popen(
self.pin_command,
stdout=subprocess.PIPE,
Copy link

@graingert graingert Mar 20, 2021

Choose a reason for hiding this comment

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

I think the stdout is captured only for logging it later, so we could just set: stdout=None (the subprocess inherits stdout) and then skip logging stdout in verbose mode

Copy link
Author

Choose a reason for hiding this comment

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

Setting the stdout=None here gives:

Traceback (most recent call last):
  File "/Users/john/.virtualenvs/pip-compile-multi-demo/lib/python3.8/site-packages/pipcompilemulti/cli_v1.py", line 26, in cli
    recompile()
  File "/Users/john/.virtualenvs/pip-compile-multi-demo/lib/python3.8/site-packages/pipcompilemulti/actions.py", line 30, in recompile
    compile_topologically(env_confs, deduplicator)
  File "/Users/john/.virtualenvs/pip-compile-multi-demo/lib/python3.8/site-packages/pipcompilemulti/actions.py", line 39, in compile_topologically
    if env.maybe_create_lockfile():
  File "/Users/john/.virtualenvs/pip-compile-multi-demo/lib/python3.8/site-packages/pipcompilemulti/environment.py", line 50, in maybe_create_lockfile
    self.create_lockfile()
  File "/Users/john/.virtualenvs/pip-compile-multi-demo/lib/python3.8/site-packages/pipcompilemulti/environment.py", line 74, in create_lockfile
    output = process.stdout.readline()
AttributeError: 'NoneType' object has no attribute 'readline'

Choose a reason for hiding this comment

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

Yeah you won't need to use process.stdout anymore as it should be sent to the terminal already

Copy link
Author

Choose a reason for hiding this comment

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

Oh my that makes it simpler

@peterdemin
Copy link
Owner

Looking good. The outstanding steps:

  • Add -v/--verbose option through a feature in features/ directory.
  • Fix logging for pip-tools failure.

@peterdemin
Copy link
Owner

Closing in favor of #251

@peterdemin peterdemin closed this Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants