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

BF: Report info from annex JSON error message in CommandError #5809

Merged
merged 3 commits into from Jul 26, 2021

Conversation

mih
Copy link
Member

@mih mih commented Jul 22, 2021

When a CommandError bubbles up to the main entrypoint it is rendered
differently than anywhere else (calls to_str() with
include_output=False. This is done to be able to write the
stdout|err output of a failed command to the proper channels. However,
this also caused all information extraction from JSON records to be
disabled.

This change, makes sure that JSON records are processed even in this
case. It also RFs the exception rendering code to make it less
repr()-like.

From:

CommandError: 'git -c diff.ignoreSubmodules=none annex copy --batch -z --to expdir --fast --json --json-error-messages --json-progress -c annex.dotfiles=true' failed with exitcode 1 under /tmp/testds
git-annex: copy: 2 failed

To:

CommandError: 'git -c diff.ignoreSubmodules=none annex copy --batch -z --to expdir --fast --json --json-error-messages --json-progress -c annex.dotfiles=true' failed with exitcode 1 under /tmp/testds [info keys: stdout_json]
> to expdir...
  remote is configured with exporttree=yes; use `git-annex export` to store content on it
  This could have failed because --fast is enabled. [2 times]
git-annex: copy: 2 failed

Fixes #5807

@mih mih linked an issue Jul 22, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #5809 (569c3f9) into maint (3bb1e93) will decrease coverage by 6.01%.
The diff coverage is 93.33%.

❗ Current head 569c3f9 differs from pull request most recent head d0b99af. Consider uploading reports for the commit d0b99af to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5809      +/-   ##
==========================================
- Coverage   90.14%   84.12%   -6.02%     
==========================================
  Files         300      297       -3     
  Lines       42353    42356       +3     
==========================================
- Hits        38180    35634    -2546     
- Misses       4173     6722    +2549     
Impacted Files Coverage Δ
datalad/support/exceptions.py 82.46% <88.23%> (-0.21%) ⬇️
datalad/cmdline/tests/test_main.py 96.62% <100.00%> (-1.91%) ⬇️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 19.35% <0.00%> (-80.65%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
datalad/metadata/extractors/exif.py 18.75% <0.00%> (-78.13%) ⬇️
datalad/metadata/extractors/image.py 19.35% <0.00%> (-77.42%) ⬇️
datalad/metadata/extractors/audio.py 20.00% <0.00%> (-77.15%) ⬇️
datalad/metadata/extractors/tests/test_exif.py 24.00% <0.00%> (-76.00%) ⬇️
datalad/metadata/extractors/tests/test_xmp.py 26.92% <0.00%> (-73.08%) ⬇️
... and 121 more

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 3bb1e93...d0b99af. Read the comment docs.

When a `CommandError` bubbles up to the main entrypoint it is rendered
differently than anywhere else (calls `to_str()` with
`include_output=False`. This is done to be able to write the
`stdout|err` output of a failed command to the proper channels. However,
this also caused all information extraction from JSON records to be
disabled.

This change, makes sure that JSON records are processed even in this
case. It also RFs the exception rendering code to make it less
repr()-like.

From:

```
CommandError: 'git -c diff.ignoreSubmodules=none annex copy --batch -z --to expdir --fast --json --json-error-messages --json-progress -c annex.dotfiles=true' failed with exitcode 1 under /tmp/testds
git-annex: copy: 2 failed
```

To:

```
CommandError: 'git -c diff.ignoreSubmodules=none annex copy --batch -z --to expdir --fast --json --json-error-messages --json-progress -c annex.dotfiles=true' failed with exitcode 1 under /tmp/testds [info keys: stdout_json]
> to expdir...
  remote is configured with exporttree=yes; use `git-annex export` to store content on it
  This could have failed because --fast is enabled. [2 times]
git-annex: copy: 2 failed
```

Fixes datalad#5807
@mih mih added the semver-patch Increment the patch version when merged label Jul 23, 2021
@mih
Copy link
Member Author

mih commented Jul 23, 2021

The new test in a single travis run is failing (does not see the desired exception output). ATM I have no idea why this would only happen in this run, and nowhere else. I restarted it, but that is mostly a desperate move.

Edit: Yeah, deterministic failure. So it seems that the git-annex version 7.20190819 that is specifically tested there, does not provide this kind of error. So I guess I should add a version check to the test.

@yarikoptic
Copy link
Member

Edit: Yeah, deterministic failure. So it seems that the git-annex version 7.20190819 that is specifically tested there, does not provide this kind of error. So I guess I should add a version check to the test.

Thanks for nailing it down! great that we have that run with that old annex. Note that in maint we have

$> grep MIN datalad/support/annexrepo.py
    GIT_ANNEX_MIN_VERSION = '7.20190503

and in master

$> grep MIN datalad/support/annexrepo.py
    GIT_ANNEX_MIN_VERSION = '8.20200309'

so if you could check that it passes on 8.20200309 - then we should strip that check away whenever merging into master

@mih
Copy link
Member Author

mih commented Jul 26, 2021

OK, all green. Good to go!

@mih mih merged commit ec839bc into datalad:maint Jul 26, 2021
@mih mih deleted the bf-5807 branch July 26, 2021 15:49
yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Aug 14, 2021
…to get msg out

By no means I did thorough investigation but

- test currently fails on cron (and locally) if I do have 8.20200309 installed

- last change to that line/logic is from later -- so I decided to go with that one

	$> git blame Remote/Helper/ExportImport.hs | grep 'use .git-annex export'
	9a2c8757f3 Remote/Helper/ExportImport.hs (Joey Hess 2020-12-18 14:52:57 -0400 153) 					then giveup "remote is configured with exporttree=yes; use `git-annex export` to store content on it"
	$> git describe --contains 9a2c8757f3
	8.20201129~30^2~22

  someone else is welcome to bisect closer if that is not the version

refs:
- prior discussion datalad#5809 (comment)
- Closes datalad#5879
yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Aug 14, 2021
…to get msg out

By no means I did thorough investigation but

- test currently fails on cron (and locally) if I do have 8.20200309 installed

- last change to that line/logic is from later -- so I decided to go with that one

	$> git blame Remote/Helper/ExportImport.hs | grep 'use .git-annex export'
	9a2c8757f3 Remote/Helper/ExportImport.hs (Joey Hess 2020-12-18 14:52:57 -0400 153) 					then giveup "remote is configured with exporttree=yes; use `git-annex export` to store content on it"
	$> git describe --contains 9a2c8757f3
	8.20201129~30^2~22

  someone else is welcome to bisect closer if that is not the version

refs:
- prior discussion datalad#5809 (comment)
- Closes datalad#5879
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CommandError info hidden (likely wide-spread issue)
2 participants