Skip to content

Set useNativeGit in git-commit-id-plugin#11365

Merged
hashhar merged 1 commit intotrinodb:masterfrom
ksobolew:kudi/git-commit-id-in-worktrees
Mar 8, 2022
Merged

Set useNativeGit in git-commit-id-plugin#11365
hashhar merged 1 commit intotrinodb:masterfrom
ksobolew:kudi/git-commit-id-in-worktrees

Conversation

@ksobolew
Copy link
Copy Markdown
Contributor

@ksobolew ksobolew commented Mar 8, 2022

Description

This is a workaround for inability to build Trino in a git worktree. This is a long-standing issue in the plugin, see git-commit-id/git-commit-id-maven-plugin#215.

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) I'm uncertain if this qualifies to release notes. This fixes an annoyance for me, but I'm not sure if it's notable enough for others.
( ) Release notes entries required with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Mar 8, 2022
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thank you. I too use worktrees a lot.

Is it possible to enable it only when not on CI though? IIRC the executable adds considerable overhead. But maybe overhead isn't so bad since IIRC @nineinchnick recently changed to make the plugin run only once instead of per-module?

@ksobolew
Copy link
Copy Markdown
Contributor Author

ksobolew commented Mar 8, 2022

Yes, I was worried about CI as well, though first I wanted to see if it works there at all (hence the draft status) :) If this plugin runs only once, then probably Maven has way larger overhead than executing a native binary, though.

@ksobolew ksobolew requested a review from nineinchnick March 8, 2022 10:14
@nineinchnick
Copy link
Copy Markdown
Member

Can you time it to see if there's a difference at all?

@ksobolew
Copy link
Copy Markdown
Contributor Author

ksobolew commented Mar 8, 2022

Can you time it to see if there's a difference at all?

We can wait and see once the build passes. But I don't think we can make any meaningful measurements.

Anyway, I could add this, just in case:

Author: Krzysztof Sobolewski <krzysztof.sobolewski@starburstdata.com>
Date:   Tue Mar 8 11:23:53 2022 +0100

    fixup! Set useNativeGit in git-commit-id-plugin

diff --git a/pom.xml b/pom.xml
index f5663e9084..4303a4c5c2 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1928,5 +1928,26 @@
                 </plugins>
             </build>
         </profile>
+        <profile>
+            <id>disable-git-worktree-workaround-in-ci</id>
+            <activation>
+                <property>
+                    <name>env.CONTINUOUS_INTEGRATION</name>
+                </property>
+            </activation>
+            <build>
+                <pluginManagement>
+                    <plugins>
+                        <plugin>
+                            <groupId>pl.project13.maven</groupId>
+                            <artifactId>git-commit-id-plugin</artifactId>
+                            <configuration>
+                                <useNativeGit>false</useNativeGit>
+                            </configuration>
+                        </plugin>
+                    </plugins>
+                </pluginManagement>
+            </build>
+        </profile>
     </profiles>
 </project>

@ksobolew ksobolew force-pushed the kudi/git-commit-id-in-worktrees branch from 9acf88b to 30e680d Compare March 8, 2022 10:28
@nineinchnick
Copy link
Copy Markdown
Member

I don't expect any difference at all either, but it would be nice to check before merging :-)

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Mar 8, 2022

We can wait and see once the build passes. But I don't think we can make any meaningful measurements.

Welcome to https://scans.gradle.com/#maven.

BTW I don't notice noticeable performance difference. (Probably because <runOnlyOnce>true</runOnlyOnce>) Good to go if this passes CI imo.

@ksobolew ksobolew marked this pull request as ready for review March 8, 2022 10:51
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 8, 2022

Is it possible to enable it only when not on CI though?

The CI should ensure things work, also for engineers.
What gains would you expect here?

This is a workaround for inability to build Trino in a git worktree.
This is a long-standing issue in the plugin, see
git-commit-id/git-commit-id-maven-plugin#215.
@ksobolew ksobolew force-pushed the kudi/git-commit-id-in-worktrees branch from 30e680d to a5f7e57 Compare March 8, 2022 13:37
@hashhar hashhar merged commit 6a63d25 into trinodb:master Mar 8, 2022
@github-actions github-actions bot added this to the 373 milestone Mar 8, 2022
@ksobolew ksobolew deleted the kudi/git-commit-id-in-worktrees branch March 9, 2022 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants