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

TST: no HOME change on OSX #6251

Merged
merged 2 commits into from
Dec 1, 2021
Merged

TST: no HOME change on OSX #6251

merged 2 commits into from
Dec 1, 2021

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Nov 26, 2021

System level configured git-credential helper osxkeychain on OSX hangs
if it can't find the Keyring under HOME, which we change to some bogus
dir for the tests. On OSX change the approach to make use of
GIT_CONFIG_GLOBAL instead of changing HOME if git version supports it (>= 2.32).

@bpoldrack bpoldrack added the semver-tests Changes only affect tests, no impact on version label Nov 26, 2021
@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #6251 (4636d13) into master (a9f423a) will increase coverage by 0.00%.
The diff coverage is 96.96%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6251   +/-   ##
=======================================
  Coverage   89.62%   89.63%           
=======================================
  Files         323      323           
  Lines       41979    42003   +24     
=======================================
+ Hits        37624    37648   +24     
  Misses       4355     4355           
Impacted Files Coverage Δ
datalad/__init__.py 87.57% <95.83%> (+0.90%) ⬆️
datalad/tests/test_base.py 100.00% <100.00%> (ø)
datalad/tests/test_config.py 100.00% <100.00%> (ø)
datalad/local/remove.py 97.01% <0.00%> (ø)
datalad/distributed/create_sibling_gin.py 83.87% <0.00%> (ø)
datalad/distributed/create_sibling_gogs.py 100.00% <0.00%> (ø)
datalad/distributed/create_sibling_gitea.py 60.52% <0.00%> (ø)
datalad/distributed/create_sibling_github.py 52.94% <0.00%> (ø)
datalad/interface/base.py 89.66% <0.00%> (+0.14%) ⬆️

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 a9f423a...4636d13. Read the comment docs.

@adswa
Copy link
Member

adswa commented Nov 26, 2021

@adswa you mention 2.30, I believe. Where did you find that info?

Git's release notes (e.g, https://github.com/git/git/blob/master/Documentation/RelNotes/2.32.0.txt), but I have to correct myself, its actually 2.32

@mih
Copy link
Member

mih commented Nov 26, 2021

Git version info is in each log output.

Copy link
Member

@adswa adswa left a comment

Choose a reason for hiding this comment

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

Cool! I was wondering about the comment and work-around in test_config about Windows. As our appveyor builds actually use Git 2.33 now, and a similar approach seems possible also for Windows. I'd suggest to remove the comment - and if the Git version on Windows is the only reason to not use the "easier approach", I could submit a follow up PR with a similar implementation for Windows and a change for this test.

datalad/tests/test_config.py Outdated Show resolved Hide resolved
@mih
Copy link
Member

mih commented Nov 26, 2021

For some reason windows is not happy with this change. The error is new, I have not yet seen it elsewhere before

System level configured git-credential helper osxkeychain on OSX hangs
if it can't find the Keyring under HOME, which we change to some bogus
dir for the tests. On OSX change the approach to make use of
GIT_CONFIG_GLOBAL instead of changing HOME.

Note, however, that this env var isn't supported by git until versions
2.32.
@bpoldrack
Copy link
Member Author

bpoldrack commented Nov 28, 2021

@mih

For some reason windows is not happy with this change. The error is new, I have not yet seen it elsewhere before

Wil try to check this, but I don't think it's this change, since a) this PR is just extracted from #5796 where it passed with this change on windows and b) this PR doesn't actually change anything on windows. However, with the new underlying image for windows, will look into it.

Edit:

FTR: Passed here: https://ci.appveyor.com/project/mih/datalad/builds/41691735

adswa added a commit to adswa/datalad that referenced this pull request Nov 29, 2021
This change sits on top of dataladgh-6251 and expands its change to
all platforms with Git version higher than 2.32. This should
ensure that we do not override the users USERPROFILE and HOME
variable by default and for all anymore (datalad#6160). Instead, only
a small set of tests use this approach, and clean up after
themselves.
@mih
Copy link
Member

mih commented Dec 1, 2021

I will shortly push an update to this PR (into this PR) -- different tooling use

@mih mih merged commit dd1c3b8 into datalad:master Dec 1, 2021
@mih mih deleted the tst-keep-home-osx branch December 1, 2021 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-tests Changes only affect tests, no impact on version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants