Declare minimum supported Git version #4650
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
In tests with Git v2.9 (which is the oldest Git version that will
build for me on Debian Buster), test_AnnexRepo_file_has_content()
fails:
File "[...]/datalad/support/tests/test_annexrepo.py", line 318, in
test_AnnexRepo_file_has_content [ar.supports_unlocked_pointers])
AssertionError: [False] != [True]
The failure is coming from the 'git diff' call unexpectedly reporting
a staged type change as modified. Adding a 'git status' call before
the 'git diff' call is enough to convince Git that there is no
modification, so perhaps it's an issue related to not triggering the
clean filter.
This failure disappears with Git v2.13.
test_duecredit() expect the output to be entirely empty when duecredit
doesn't emit anything. At least one time when that isn't the case is
when running with an older Git:
AssertionError: '[...]/datalad/support/gitrepo.py:978:
OutdatedExternalDependencyWarning: Your git version (2.13.0) is
too old, we will not safe-guard against creating a new repository
under already known to git subdirectory\n
OutdatedExternalDependencyWarning\n' != ''
MissingExternalDependency already tacks on a period to its str value,
so unless the caller provides more text via msg, the output has an
extra period:
datalad.support.exceptions.OutdatedExternalDependency: cmd:git of
version >= 2.19.1 is missing.. You have version 2.13.0
An extra period could also end up in the output when a caller
specifies msg, but none of the callers at the moment end their message
with a period.
'git config' doesn't raise the expected error until 2.13.2, specifically 25cd291963 (config: complain about --local outside of a git repo, 2017-05-12).
This test depends on a change that came with v2.13.2: fdb69d33c4 (fetch-pack: always allow fetching of literal SHA1s, 2017-05-15).
We do not specify a minimum supported Git version, which makes it difficult for us to decide whether we can use a newer feature and means that users have to rely on trial and error to know whether their version is recent enough. Set a minimum Git version, handling it in a way that closely follows what AnnexRepo already does for the minimum git-annex version. Choose 2.19.1 (September 2018) as the minimum because that (1) matches what conda currently has for its 32-bit Linux builds [0] and (2) is below what is currently in Debian stable (v2.20.1). Note that if we decide to lower the minimum more, the hard floor should probably be considered v2.13.0 (May 2017). As described a few commits back, AnnexRepo._check_files() doesn't work reliably until then and testing with v2.13.0 didn't reveal major problems [1], though the skips added in the two previous commits were needed. [0] datalad#4636 (comment) [1] https://travis-ci.org/github/datalad/datalad/builds/701067382 Closes datalad#4636.
With the previous commit, we now specify and verify a minimum Git version of 2.19.1.
external_versions["cmd:git"] is now stored as GitRepo.git_version. Use that in a few tests.
Codecov Report
@@ Coverage Diff @@
## master #4650 +/- ##
==========================================
+ Coverage 89.60% 89.62% +0.01%
==========================================
Files 288 288
Lines 39023 39017 -6
==========================================
+ Hits 34966 34968 +2
+ Misses 4057 4049 -8
Continue to review full report at Codecov.
|
Since 0.13.0 isn't out yet, and #4620 was just merged (so fresh code just arrived to rc), I would be ok to see this in 0.13.0.
I wouldn't bother with warnings at different versions/ levels and just have exception (as now) . Our minimal version is indeed far back enough |
Thanks for taking a look.
Yeah, perhaps it's true that our minimum is low enough, though I'm less confident that people aren't running with older git versions. The base of this PR is suitable for both master (now targeting 0.14.0) and maint (now targeting 0.13.1). So, given nobody else has chimed in since gh-4636 was opened with other suggestions for a minimum version, I'll merge this into master now. With the base where it is, we can easily merge this into maint as well if we decide to, though if we do so I still lean towards switching to a warning there. |
|
users keep running into this... @kyleam - would you be kind to furnish a PR also to |
kyleam
added a commit
to kyleam/datalad
that referenced
this pull request
Sep 3, 2020
kyleam
added a commit
to kyleam/datalad
that referenced
this pull request
Sep 3, 2020
The master branch (what will be 0.14) already contains the previous commits in this series, declaring a minimum git version of 2.19.1 and aborting if we detect something below it. As described in ff042b8 (ENH: gitrepo: Declare and check minimum Git version, 2020-06-22), this minimum version is forward-looking and based off what's available from particular distributions. The first Git release with a known issue is actually much earlier, v2.13. Merging this series to the maint branch as is then would be too aggressive for a maintenance release because no prior release has declared a minimum version, yet we'd be making DataLad non-functional with a range of Git versions that don't have known issues. And even raising an error if Git is below v2.13 could be problematic because the subset of functionality that a user relies on may not be affected. Instead issue a warning if the detected Git is below v2.13.0 to let users that run into problems know that their Git version is likely the culprit. Re: datalad#4650
yarikoptic
added a commit
to yarikoptic/datalad
that referenced
this pull request
Mar 7, 2023
As @bpoldrack identified in datalad#7291 (comment) the 629e575 which was part of the datalad#4650 concerning introduction of minimal git version to be 2.19.1, and not removing that condition was was combined with argument to the merge function. That commit was released within good old 0.13.4. This change merely reintroduces "if" statement BUT unfortunately that allow_unrelated nowhere exposed in the API really, and used only once with allow_unrelated=True in some single test. So I expect some tests to fail here and most likely us needing to introduce similar option in "update" to finalize this PR.
yarikoptic
added a commit
to yarikoptic/datalad
that referenced
this pull request
Mar 16, 2023
As @bpoldrack identified in datalad#7291 (comment) the 629e575 which was part of the datalad#4650 concerning introduction of minimal git version to be 2.19.1, and not removing that condition was was combined with argument to the merge function. That commit was released within good old 0.13.4. This change merely reintroduces "if" statement BUT unfortunately that allow_unrelated nowhere exposed in the API really, and used only once with allow_unrelated=True in some single test. So I expect some tests to fail here and most likely us needing to introduce similar option in "update" to finalize this PR.
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 series sets a minimum Git version (currently v2.19.1, as suggested in gh-4636), and adds a cron build to test against it.
Make sure run with v2.19.1 passes and then drop the tip commit.
v2.19.1 will hopefully pass because v2.13.0 fared okay, though the tests needed a couple of skips for behavior that changed with v2.13.2. The idea with that test is to get an idea of what our functional minimal version is at this point, regardless of whether we choose a higher floor. I chose v2.13.0 because it is the earliest version that I didn't find issues with when testing locally.
https://travis-ci.org/github/datalad/datalad/builds/701074711
Decide where to introduce the minimal version. As I mentioned in We need to establish a minimal supported git version and puke informatively #4636, I'd prefer not to introduce a hard floor in a bug fix release, as it runs the risk of making datalad not work at all for users where a subset of functionality may have been working fine. And we've gone a long time without specifying a minimum requirement.
My current thinking is that we should target this series for 0.13.1. Instead of aborting if an unsupported version is detected, we should just tell the user via
warnings.warnthat v2.19.1 (or whatever version we end up deciding to go with) is the oldest version we test against. If their version is below v2.13.0, we might want to give a bit stronger warning, given there are known issues at that point.Then, on the master branch (so targeting 0.14), we can switch the warning over to the
external_versions.checkcall that is currently in this PR.