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: set LC_ALL=C while running git #4342

Merged
merged 3 commits into from Mar 27, 2020
Merged

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Mar 25, 2020

That should ensure that error messages are hints are in English so
we could still parse them and react accordingly.

Closes #4340

TODO:

  • [DID NOT WORK] test - should we set LANG in some of the travis runs?

That should ensure that error messages are hints are in English so
we could still parse them and react accordingly.
@codecov
Copy link

@codecov codecov bot commented Mar 25, 2020

Codecov Report

Merging #4342 into maint will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4342      +/-   ##
==========================================
- Coverage   89.67%   89.55%   -0.12%     
==========================================
  Files         275      275              
  Lines       36894    36931      +37     
==========================================
- Hits        33084    33073      -11     
- Misses       3810     3858      +48     
Impacted Files Coverage Δ
datalad/cmd.py 89.68% <100.00%> (+0.05%) ⬆️
datalad/downloaders/http.py 72.11% <0.00%> (-2.79%) ⬇️
datalad/utils.py 83.97% <0.00%> (-2.67%) ⬇️
datalad/downloaders/tests/test_http.py 58.79% <0.00%> (-2.17%) ⬇️
datalad/distribution/dataset.py 95.88% <0.00%> (+0.01%) ⬆️
datalad/core/local/create.py 94.77% <0.00%> (+0.03%) ⬆️

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 cc7e627...d18ec7e. Read the comment docs.

kyleam
kyleam previously approved these changes Mar 25, 2020
Copy link
Contributor

@kyleam kyleam left a comment

Thanks.

test - should we set LANG in some of the travis runs?

I'd be okay with that being adding to an existing run, but I'm also fine with this as is.

@kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 25, 2020

I said:

[...] but I'm also fine with this as is.

Oh, but it looks like the tests are not (example).

Apparently specifying LC_ALL=C causes git-annex to freak out on unicode paths.
So now we will try more targetted approach of specifying that messages (only)
should be "C" but then popping LC_ALL so it does not overrule our setting
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 26, 2020

"interesting". Either it just fails early enough into the test, or it is specially git annex init ( or likely - any git annex command ) which can't operate under a path with unicode characters in it if LC_ALL=C. Something starts nagging in the back of my head about some old discussion with joeyh about locales&git-annex which might be relevant ;-)

I thought that we might want to just overload LC_MESSAGES but then (which seems to work ok and takes precedence over LANG), if LC_ALL is set, we are back to the original issue. So, pushed e2d177b which sets LC_MESSAGES and pops LC_ALL ;) let's see how that one works out (seems ok on a quick local testing)

@yarikoptic yarikoptic dismissed kyleam’s stale review Mar 26, 2020

It was failing tests, no good. Removing so I could get a fresh approval ;)

@kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 26, 2020

Nearly all the builds were failing in the setup phase (pypi download issue, I think). On restart, the extensions can't seem to check out the right state, but Travis and AppVeyor got to the actual tests and are running now.

kyleam
kyleam approved these changes Mar 26, 2020
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 26, 2020

hoorray -- travis and appveyor are green! Good base -- now I will try adding LC_ALL and LANG (for the sake of it) into one of the travis runs, and see how things look after that.

@@ -40,6 +40,8 @@ matrix:
# We cannot have empty -A selector, so the one which always will be fulfilled
- NOSE_SELECTION=
- NOSE_SELECTION_OP=not
# To test https://github.com/datalad/datalad/pull/4342 fix
- LC_ALL=ru_RU.UTF-8
Copy link
Contributor

@kyleam kyleam Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't you have to generate these locales?

Copy link
Member Author

@yarikoptic yarikoptic Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question on either they are in effect. Let me do one more run with a TEMP commit reverting the changes to code -- then things better fail

Copy link
Contributor

@kyleam kyleam Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thanks

Copy link
Contributor

@kyleam kyleam Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me do one more run with a TEMP commit reverting the changes to code -- then things better fail

Looking green.

Copy link
Member Author

@yarikoptic yarikoptic Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-( so indeed need to install/generate them I guess. will try now

.travis.yml Outdated Show resolved Hide resolved
@kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 27, 2020

The build still wasn't showing the expected failure. I though that might be due to the use of the bundled git. In 216204b I set DATALAD_USE_DEFAULT_GIT=1 but the build still passes, and even a plain git status call is still in English. Stumped.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 27, 2020

bundled -- indeed, it comes with its own locales... but I am using git-annex-standalone locally as well and can trigger it.
I am ok to give up on trying to make testing "work" ;) I would have probably left those two LC_ and LANG in just in case they would trigger at some point.

@kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 27, 2020

I am ok to give up on trying to make testing "work" ;)

I am too (not surprisingly, since initially I said I'm okay without a test here).

I would have probably left those two LC_ and LANG in just in case they would trigger at some point.

I'm a bit confused by this comment. It makes it sound like I pushed an update that removed them.

Anyway, I personally would prefer not to leave such cruft in the final state, but doing so is fine by me assuming there's an accompanying comment.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 27, 2020

I would have probably left those two LC_ and LANG in just in case they would trigger at some point.

I'm a bit confused by this comment. It makes it sound like I pushed an update that removed them.

nah, I meant to say that I would have taken 48c49fc as the state to merge

Anyway, I personally would prefer not to leave such cruft in the final state, but doing so is fine by me assuming there's an accompanying comment.

comment in the commit or in .travis.yml?

@kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 27, 2020

comment in the commit or in .travis.yml?

My preference would be a comment in .travis.yml for visibility and to prevent anyone needing to dig to find out that these didn't have an effect from the start.

@kyleam
Copy link
Contributor

@kyleam kyleam commented Mar 27, 2020

nah, I meant to say that I would have taken 48c49fc as the state to merge

"would have" suggests that something different has already been done.

Seems to have no effect on travis (i.e. doesn't fail without fix), but since
shouldn't hurt -- keeping it in .travis.yml just in case
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 27, 2020

ok, done -- if still green, we could merge

@yarikoptic yarikoptic added the merge-if-ok label Mar 27, 2020
@kyleam kyleam merged commit 841480c into datalad:maint Mar 27, 2020
9 of 10 checks passed
@yarikoptic yarikoptic deleted the bf-git-lang branch Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants