search

con / duct

A helper to run a command, capture stdout/stderr and details about running
https://pypi.org/project/con-duct/
MIT License
4 stars 2 forks source link

Persistently open usage file until the end and open info as "w" not "a" #209

Closed yarikoptic closed 1 month ago

yarikoptic commented 1 month ago

Closes #208

TODOs

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.05%. Comparing base (bfc9355) to head (f1d0212). Report is 12 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #209 +/- ## ========================================== + Coverage 94.00% 94.05% +0.05% ========================================== Files 2 2 Lines 567 572 +5 Branches 65 66 +1 ========================================== + Hits 533 538 +5 Misses 22 22 Partials 12 12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yarikoptic commented 1 month ago

FTR pypy 3.9 fail is that 

E                     File "/opt/hostedtoolcache/PyPy/3.9.19/x64/lib/pypy3.9/subprocess.py", line 507, in run
E                       stdout, stderr = process.communicate(input, timeout=timeout)
E                   ValueError: not enough values to unpack (expected 2, got 0)
asmacdo commented 1 month ago

@yarikoptic FWIW I see that failure, but it looks caused by the same issue we've started seeing recently. This one might be flake, and might benefit from the retry logic I've started implementing in https://github.com/con/duct/pull/202/ (though I'm still stuck on that)

test_signal_exit

            if check and retcode:
>               raise CalledProcessError(retcode, process.args,
                                         output=stdout, stderr=stderr)
E               subprocess.CalledProcessError: Command 'ps aux | grep '[s]leep 60.74016230000801'' returned non-zero exit status 1.
asmacdo commented 1 month ago

@yarikoptic I tried removing the open/close in prepare_paths, but its not simply fixing tests as I thought.

The complication is that TailPipe does require that the stdout/stderr files already exist prior to starting the inner command. 

    def start(self) -> None:
        self.stop_event = threading.Event()
>       self.infile = open(self.file_path, "rb")
E       FileNotFoundError: [Errno 2] No such file or directory: '.duct/logs/2024.10.24T10.08.47-284323_stdout'

We might be able to address that, but given that def fileno depends on the file previously existing, we might run into trouble.

Since PR will prevent the usage close/open cycle which is the most egregious, so I think its worth merging as is.

yarikoptic commented 1 month ago

agree -- let's merge/release this fix . For the rest