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

Don't fail the build if we're in a git worktree folder #6169

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

lbergelson
Copy link
Member

@lbergelson lbergelson commented Sep 18, 2019

  • Expand the way the build.gradle checks if we're in a git repo to also understand git-worktree checkouts

This is useful to me since I've been using git worktree to maintain copies of gatk for java 8 and 11 separately.

Also, I recommend git worktree

* Expand the way the build.gradle checks if we're in a git repo to also understand git-worktree checkouts
@lbergelson lbergelson requested a review from cmnbroad September 18, 2019 18:53
Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

I've never tried these since I think I would get confused - I prefer to be confused by stash. Seems reasonable enough though - just a couple of minor comment/text suggestions.

@@ -125,6 +125,11 @@ def resolveLargeResourceStubFiles(largeResourcesFolder, buildPrerequisitesMessag
}
}


def looksLikeWereInAGitRepository(){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth putting a comment here saying that its looking for either a git-dir or a worktree (or whatever the right names are) - I always get them mixed up.

@@ -134,7 +139,7 @@ def ensureBuildPrerequisites(requiredJavaVersion, largeResourcesFolder, buildPre
"The ClassLoader obtained from the Java ToolProvider is null. "
+ "A Java $requiredJavaVersion JDK must be installed. $buildPrerequisitesMessage")
}
if (!file(".git").isDirectory()) {
if (!looksLikeWereInAGitRepository()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest changing the message below to something like:

"This doesn't appear to be a git folder. The GATK Github repository must be cloned using \"git clone\" to run the build. + "$buildPrerequisitesMessage")

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

Minor comments.

@lbergelson lbergelson merged commit 333610a into master Sep 19, 2019
@lbergelson lbergelson deleted the lb_update_git_check branch September 19, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants