run: Fix path handling for gh-3746 and for subdirectory runs #3747
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sadly it does: https://github.com/datalad/datalad/pull/3747/checks?check_run_id=248506560#step:8:254 |
mih
added a commit
to mih/datalad
that referenced
this pull request
Oct 5, 2019
Associated PR in progress: datalad#3747
This tries to be a gentle RF that doesn't change (much) of the original behavior (see test diff). However, a change in-line with datalad#3742 is unavoidable (see removed test). This is bringing path semantics in line with status/subdatasets, and is a precondition to fixing datalad#3469
Also update that filter to match the new conventions
Associated PR in progress: datalad#3747
This test leads to run() calling `get(dataset=ds.path, path=['*'])`.
On non-Windows systems, the '*' is considered a literal path and
reported to be absent, as expected. On Windows, the underlying
os.stat() call fails [0] with
OSError: [WinError 123] The filename, directory name, or volume
label syntax is incorrect:
'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\datalad_temp_5m0lh8wt\\*'
As discussed here [1], we might want to rework run() and/or
GlobbedPaths to avoid hanging on to the unmatched glob and feeding
them as a literal paths. That might get rid of the failure for this
particular test, though it would just paper over the core Windows path
handling problem shown by this failure..
[0]: https://github.com/datalad/datalad/pull/3746/checks?check_run_id=248850583#step:8:417
[1]: datalad#3746 (comment)
We'll use this in another spot.
The result records are no longer based on annotate_paths.
Th next commit will pass None as the `path` argument instead of ".", which would be a breaking change to how the `saver` argument is. But adding a compatibility kludge for outside callers isn't worth the effort. This argument was added so that the datalad-revolution extension could use rev_save() instead of add() as the saver, and that code has been absorbed into the core. So let's just ignore the `saver` argument entirely and slate it for removal.
We pass "." as the path argument to Save() to mean "save all changes". The next commit will change the Save() call from a dataset-bound call to an unbound call. That means that Save(dataset=<string>, path=".") when called in a subdirectory will just save the subdirectory changes. Instead we need to pass path=None.
We already access this in a couple of spots, and the next commit will add several more.
As described here [1], there are two approaches to dealing with the current path rules: (1) resolve all paths to absolute paths and use bound dataset methods or (2) use unbound dataset methods and relative paths. run() has mostly taken the first approach. There is one spot that incorrectly passes relative paths to install() [2]. If this is changed to use absolute paths, it reveals a deeper incompatibility with the new path handling on master. rev_resolve_path() isn't happy with absolute paths that look like "/go/../upstairs" (see its docstring and code comments for why). run() constructs paths like these when the run is happening in a subdirectory and there are inputs or outputs upstairs (see the examples in the added test). So instead let's switch to the second approach. Note that the compatibility kludge around the dataset handling is mainly for the sake of datalad-htcondor. Also note that needing to use chpwd() in several spots is ugly. We do this because we need to make sure we're in the correct directory when rerunning. We might want to make rerun() responsible for calling chpwd() around its call of run_command(), but that would take a bit more refactoring. Fixes datalad#3551. Re: datalad#3746 [1] datalad#3746 (comment) [2] ... though these incorrect still work in at least some cases. See dataladgh-3650.
Codecov Report
@@ Coverage Diff @@
## master #3747 +/- ##
=========================================
- Coverage 82.94% 82.9% -0.05%
=========================================
Files 273 273
Lines 35943 35940 -3
=========================================
- Hits 29813 29795 -18
- Misses 6130 6145 +15
Continue to review full report at Codecov.
|
mih
approved these changes
Oct 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This sits on top of gh-3746 and should fix the run failures (including hopefully the Windows one; edit: that was too hopeful). Here's the restricted diff. This series could be merged into that PR or merged into master following that PR. Or, if desired, I can separate this series from that one, though there will end up being at least be some minor conflicts to deal with.
This fixes gh-3551. The added test, based off of the examples in that issue, will run on Windows, so it might need a compatibility fixup.