Fixing test failures with recent annex #6550
Merged
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
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
be1c9d1 to
a3b35fd
Compare
yarikoptic
reviewed
Mar 15, 2022
|
I guess some if not all could/should go against |
bpoldrack
force-pushed
the
fix-for-new-annex
branch
2 times, most recently
from
7fe29ca to
0848f90
Compare
Likely. Will see once I have it all. Just want to collect and debug here. |
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
7004a99 to
d551fb4
Compare
bpoldrack
force-pushed
the
fix-for-new-annex
branch
2 times, most recently
from
8aef920 to
140afc0
Compare
bpoldrack
added
the
team-gitannex
|
While the original failures are gone, I am now seeing two new failures. As of now I am failing to see how they could be related to this PR, but I checked One exclusively on github action macOS(brew) only: Which seems wild, since the Resolved by using Edit: |
|
The other one only happens on AppVeyor (macOS again): This seems flaky. Logging into that AppVeyor build, showed, that this happens at different spots in this test. Looking into this, I am seeing a Broken Pipe Error: And hanging at this point And, of course, there's no problem running this right afterwards: Edit: Same here. Implausible to be related (it's |
bpoldrack
added a commit
to bpoldrack/datalad
that referenced
this pull request
Mar 19, 2022
Seeing a strange `FileExistsError` raised by `mkdir` on a macOS CI build, when the call was conditioned on `os.path.lexists` being `False`. See: datalad#6550 (comment) Appears to be fixed by this patch.
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
0609a48 to
6f86a1b
Compare
mih
reviewed
Mar 20, 2022
This was referenced Mar 21, 2022
bpoldrack
added a commit
to bpoldrack/datalad
that referenced
this pull request
Mar 21, 2022
Seeing a strange `FileExistsError` raised by `mkdir` on a macOS CI build, when the call was conditioned on `os.path.lexists` being `False`. See: datalad#6550 (comment) Appears to be fixed by this patch.
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
6f86a1b to
01e9a71
Compare
yarikoptic
reviewed
Mar 21, 2022
bpoldrack
added a commit
to bpoldrack/datalad
that referenced
this pull request
Mar 22, 2022
Seeing a strange `FileExistsError` raised by `mkdir` on a macOS CI build, when the call was conditioned on `os.path.lexists` being `False`. See: datalad#6550 (comment) Appears to be fixed by this patch.
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
01e9a71 to
18c1ade
Compare
|
Need to test latest adjustments, hence converting back to draft mode to prevent premature merging. |
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
a476b1e to
08f81ee
Compare
bpoldrack
added a commit
to bpoldrack/datalad
that referenced
this pull request
Mar 29, 2022
Seeing a strange `FileExistsError` raised by `mkdir` on a macOS CI build, when the call was conditioned on `os.path.lexists` being `False`. See: datalad#6550 (comment) Appears to be fixed by this patch.
bpoldrack
force-pushed
the
fix-for-new-annex
branch
3 times, most recently
from
9bb6919 to
99ae79a
Compare
This was referenced Mar 30, 2022
bpoldrack
force-pushed
the
fix-for-new-annex
branch
2 times, most recently
from
7c433d4 to
f29a2bf
Compare
Skip over a narrow range of annex versions, that have the bug as reported at https://git-annex.branchable.com/bugs/Change_to_annex.largefiles_leaves_repo_modified/. (Closes datalad#6570)
As of annex' 10.20220127 an issue became apparent (actually older), where the reporting of annex commands with respect to paths that are unknown to git or not existent at all happens in different ways. This depends on annex' internal git calls (git itself appears to be inconsistent WRT this message) as well as on the value of 'annex.skipunknown' the default of which changed in said version. This results in both styles of reporting being used and as long as we rely on parsing that output, we need to account for both. Moreover, annex commands don't always exit non-zero when that happens, which is why we need to check stderr output when the command doesn't fail, too. Fixed annex (ce91f10132805d11448896304821b0aa9c6d9845, Feb 28, 2022 - "fix annex.skipunknown false error propagation"), which is part of version 10.20220322. None of that actually depends on annex' version exactly. It's mostly just that the zero-exit bug in combination with the switch of default made those issues surface. (Closes datalad#6492) (Closes datalad#6571)
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
f29a2bf to
88745e2
Compare
With changed reporting on unknown to git paths by annex (which is based
on `git ls-files --error-unmatch`) let datalad commands report those
errors, rather than crashing or completely ignoring them.
This will change behavior of some datalad commands. The change partially
only applies to annex version >= 10.20220322 and annex.skipunknown=false
(default since 10.20220127).
Assume following setup:
.
├── bar -> foo
├── bla
├── foo
│ └── f -> ../.git/annex/objects/ ...
└── untracked
Then, previously we had (all zero-exit):
$ datalad status --annex availability notexistent foo/f
1 annex'd file (0.0 B/0.0 B present/total size)
nothing to save, working tree clean
$ datalad status --annex availability bar/f foo/f
1 annex'd file (0.0 B/0.0 B present/total size)
nothing to save, working tree clean
$ datalad drop notexistent
$ datalad drop bar/f
$ datalad diff -f HEAD~2 --annex availability bar/f notexistent foo/f
added: foo/f (file)
Now, with non-zero exit:
$ datalad status --annex availability notexistent foo/f
status(error): notexistent [File unknown to git]
1 annex'd file (0.0 B/0.0 B present/total size)
$ datalad status --annex availability bar/f foo/f
status(error): bar/f [File unknown to git]
1 annex'd file (0.0 B/0.0 B present/total size)
status(error): bar/f [File unknown to git]
$ datalad drop notexistent
drop(error): notexistent (file) [File unknown to git]
$ datalad drop bar/f
drop(error): bar/f (file) [File unknown to git]
$ datalad diff -f HEAD~2 --annex availability bar/f notexistent foo/f
added: foo/f (file)
unknown: bar/f ()
unknown: notexistent ()
Note, that this only has an effect when the command goes through
`AnnexRepo.get_content_annexinfo`. Calls, that are only assessing things
based on plain git are not affected. This may be desirable to add,
though, since the reporting is somewhat similar.
bpoldrack
force-pushed
the
fix-for-new-annex
branch
from
88745e2 to
15a30a4
Compare
|
Code Climate has analyzed commit 15a30a4 and detected 0 issues on this pull request. View more on Code Climate. |
1 task
Codecov Report
@@ Coverage Diff @@
## master #6550 +/- ##
==========================================
+ Coverage 91.13% 91.17% +0.03%
==========================================
Files 350 351 +1
Lines 44232 44272 +40
==========================================
+ Hits 40312 40364 +52
+ Misses 3920 3908 -12
Continue to review full report at Codecov.
|
yarikoptic
approved these changes
Apr 1, 2022
There was a problem hiding this comment.
CI is happy (ish) -- I am happy, although would be nice to test it out on recent snapshot. I have asked @joeyh about oddity with OSX snapshot and its versioning. But whenever we merge this, we will see effects in testing of datalad/git-annex. I will also add a label for a potential CP into maint to get that one green too!
| @@ -3297,12 +3311,23 @@ def get_content_annexinfo( | |||
| path = self.pathobj.joinpath(ut.PurePosixPath(j['file'])) | |||
| rec = info.get(path, None) | |||
| if rec is None: | |||
| if init is not None: | |||
| # git didn't report on this path | |||
| if j.get('success', None) is False: | |||
There was a problem hiding this comment.
I guess should be of the same effect but the pattern we used already is
Suggested change
| if j.get('success', None) is False: | |
| if not j.get('success', True): |
which avoids ising bools .
|
Let's proceed , thank you @bpoldrack for taking care about it! |
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.
As of annex 10.20220127 we observed several failures on macOS, which at this point is the only build with that recent an git-annex.
These failures boil down to two issues:
One issue became apparent (actually older), where the reporting of annex commands with respect to paths that are
unknown to git or not existent at all happens in different ways. This depends on annex' internal git calls (git itself appears to be inconsistent WRT this message) as well as on the value of 'annex.skipunknown' the default of which changed in said version. This results in both styles of reporting being used and as long as we rely on parsing that output, we need to account for both.
Moreover, annex commands don't always exit non-zero when that happens, which is why we need to check stderr output when the command doesn't fail, too. At least for
git-annex-findbut probably more commands the zero exit is fixed but unreleased in annex (10.20220222-30-gce91f1013, Feb 28, 2022 - "fix annex.skipunknown false error propagation").None of that actually depends on annex' version exactly. It's mostly just that the zero-exit bug in combination with the switch of default made those issues surface.
The other issue seems to be an annex bug: https://git-annex.branchable.com/bugs/Change_to_annex.largefiles_leaves_repo_modified/
This patch simply skips the respective part of the test for this situation conditioned on the annex version.
Closes #6492
Closes #6570
Closes #6571
Please, review especially this change, @datalad/developers :
This will change behavior of some datalad commands. The change partially
only applies to annex version >= 10.20220322 and
annex.skipunknown=false(default since 10.20220127).
Assume following setup:
.
├── bar -> foo
├── bla
├── foo
│ └── f -> ../.git/annex/objects/ ...
└── untracked
Then, previously we had (all zero-exit):
Now, with non-zero exit:
Note, that this only has an effect when the command goes through
AnnexRepo.get_content_annexinfo. Calls, that are only assessing thingsbased on plain git are not affected. This may be desirable to add,
though, since the reporting is somewhat similar.
Also note, that we can't distinguish the
bar/fandnotexistentcase anymore, since this is reported on equally by git-annex, which in turn now relies ongit ls-files --error-unmatch.Changelog
🐛 Bug Fixes
🛡 Tests