-
Notifications
You must be signed in to change notification settings - Fork 11
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
Include copy of https://github.com/uktrade/iterable-subprocess #538
Conversation
git-subtree-dir: datalad_next/iterable_subprocess git-subtree-split: 46343c074d2c062dd0822e60a86665b2cce2d93b
…ext/iterable_subprocess' Source: https://github.com/uktrade/iterable-subprocess
This comment was marked as resolved.
This comment was marked as resolved.
It is not 100% clear to me whether this is correct, but `OSError(errno22)` can and does occur when interacting with file descriptors of dead processes.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #538 +/- ##
==========================================
+ Coverage 91.24% 92.66% +1.41%
==========================================
Files 128 137 +9
Lines 9641 10031 +390
Branches 1045 1085 +40
==========================================
+ Hits 8797 9295 +498
+ Misses 825 712 -113
- Partials 19 24 +5 ☔ View full report in Codecov by Sentry. |
This is ready. I will now port previous attempts to this new implementation -- in separate PRs. |
#539 has now shown that this code is both fast and usable. I will now add a thin wrapper around it, such that we can extend without changing its nature too much (possibly ease future merges). |
That module imports too many components with too many dependency that also needs (these) constants. This creates import loops. The change established a toplevel `consts` module with minimal dependencies. The legacy location is kept and a comment warns against further use.
This is a convenience wrapper around `datalad_next.iterable_subprocess`. The main purpose is to have a place for documentation and feature additions -- without adding needless diff to the original implementation that was merged in from an external source. The wrapper uses `COPY_BUFSIZE` as default chunk size (which is numerically identical to the hardcoded default on POSIX systems). This should be better than a separate literal. A second different is the option to run a subprocess without any explicit input.
There is not really a point to compatibility with versions that are nearing EOL
For now containers processors for byte-string iterables and functionality related to subprocess output.
OK, let's do this! |
This brings the preconditions for #539, which in itself has the evidence for the utility of this approach.
Historic context from here.
ATM` it is still unclear whether it will be adopted. This changeset is primarily a preparation for running the tests in all contexts that we need.
The sources where included via
Closes #536
Closes #537
TODO:
ls
,cat
,funzip
. The first two should be replacable withdir
andtype
, but the latter (from unzip) might need to become a conditional for running the test. It is installed on appveyor, though.SOLVED: All tools, also
funzip
come with Git (also on Windows). It may need a PATH adjustment on a particular machine, but not additional install.OSError Errno 22
. The conditions under which this happens are not completely clear to me right now. Simply catchingOSError
seems not enough.Test log