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

Trino can't build from a git worktree #18027

Closed
xkrogen opened this issue Jun 23, 2023 · 7 comments · Fixed by #18046
Closed

Trino can't build from a git worktree #18027

xkrogen opened this issue Jun 23, 2023 · 7 comments · Fixed by #18046

Comments

@xkrogen
Copy link
Member

xkrogen commented Jun 23, 2023

A while ago a change was made (6a63d25) to change the git-commit-id-maven-plugin, pulled from https://github.com/airlift/airbase, to use the native git implementation to work around an issue where the jGit implementation used by the plugin is incompatible with git worktrees (git-commit-id/git-commit-id-maven-plugin#215). This worked fine up until recently when airbase was upgraded from 139 to 141 (f769e9d). As part of this, the version of git-commit-id-maven-plugin was changed from 5.0.0 to 6.0.0 (airlift/airbase#359), which introduced the issue described in git-commit-id/git-commit-id-maven-plugin#639 where the native git workaround doesn't work out of the box, because the working directory for git commands is incorrectly detected in a worktree environment. It's possible to work around this by explicitly adjusting the .git directory used by the plugin:

> git diff | cat
diff --git a/pom.xml b/pom.xml
index bc079ac5cc..8d6ca7b576 100644
--- a/pom.xml
+++ b/pom.xml
@@ -2089,6 +2089,7 @@
                         <injectAllReactorProjects>true</injectAllReactorProjects>
                         <!-- A workaround to make build work in a Git worktree, see https://github.com/git-commit-id/git-commit-id-maven-plugin/issues/215 -->
                         <useNativeGit>true</useNativeGit>
+                        <dotGitDirectory>${project.basedir}/../../../.git</dotGitDirectory>
                     </configuration>
                 </plugin>
                 <plugin>

That workaround seems to work okay even when Trino is checked out without a worktree, but it's quite a hack, so I'd like to see if anyone has better ideas before opening a PR for it.

@xkrogen
Copy link
Member Author

xkrogen commented Jun 23, 2023

cc @ksobolew since you made the change in 6a63d25

@xkrogen
Copy link
Member Author

xkrogen commented Jun 23, 2023

cc also @wendigo as FYI since you did the version upgrade of the plugin in airbase

@ksobolew
Copy link
Contributor

+                        <dotGitDirectory>${project.basedir}/../../../.git</dotGitDirectory>

Well, the number of ..'s required is different for some modules, so this won't work in general. But I think every module has the same parent, so maybe using parent's base dir would work?

@ksobolew
Copy link
Contributor

ksobolew commented Jun 26, 2023

But in general, any number of horrible hacks is justified if there's no other way and we clearly mark it as technical debt to be paid off later.

@wendigo
Copy link
Contributor

wendigo commented Jun 26, 2023

I'd prefer to fix the plugin rather then hack the usage in Trino

@ksobolew
Copy link
Contributor

Me too, but look how that's going for the original issue :)

@xkrogen
Copy link
Member Author

xkrogen commented Jun 26, 2023

Well, the number of ..'s required is different for some modules, so this won't work in general. But I think every module has the same parent, so maybe using parent's base dir would work?

@ksobolew This did seem to work (my build completed successfully), but I also was not expecting it to. Perhaps I got lucky about which modules are actually using this plugin, or something like that? Not sure.

In any case I discovered ${maven.multiModuleProjectDirectory} which actually represents what we are looking for. From MNG-6589 it seems like this is supposed to be a private implementation detail (to be publicly exposed in Maven 4.0.0; see MNG-7038). But our mvwn explicitly sets this so I am not too worried about relying on it until we use Maven 4.0:

trino/mvnw

Line 315 in 8217bb3

"-Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR}" \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants