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

build: Fix building clboss as a git submodule #250

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

chrisguida
Copy link
Contributor

@chrisguida chrisguida commented Oct 24, 2024

When clboss is a git submodule, the current Makefile fails because it assumes that, because the $(srcdir)/.git/logs/HEAD file does not exist, we must be inside the release zip and therefore already have the commit_hash.h file present.

Here we update the Makefile to detect the case where clboss is a git submodule (by detecting that .git is a regular file and not a directory), and run generate_commit_hash.sh in that case.

Fixes #247

@ksedgwic
Copy link
Collaborator

@chrisguida I found an alternative which uses git itself to decide (and fails appropriately if git is missing):

diff --git a/Makefile.am b/Makefile.am
index 7f31dff..1e3d4b7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -679,8 +679,10 @@ create-tarball: create-tarball.in Makefile
 BUILT_SOURCES = commit_hash.h
 
 commit_hash.h: FORCE
-	@if test -e $(srcdir)/.git/logs/HEAD; then \
-		if [ ! -f $(srcdir)/commit_hash.h ] || [ $(srcdir)/.git/logs/HEAD -nt $(srcdir)/commit_hash.h ]; then \
+	@if git -C $(srcdir) rev-parse --is-inside-work-tree > /dev/null 2>&1; then \
+		CURRENT_HASH="$$(git -C $(srcdir) rev-parse HEAD)"; \
+		if [ ! -f $(srcdir)/commit_hash.h ] || \
+           [ "$$(grep -oE '[0-9a-f]{40}' $(srcdir)/commit_hash.h)" != "$$CURRENT_HASH" ]; then \
 			echo "Regenerating $(srcdir)/commit_hash.h..."; \
 			$(SHELL) $(srcdir)/generate_commit_hash.sh; \
 		else \

WDYT?

@chrisguida
Copy link
Contributor Author

@ksedgwic yes, this works, as long as @Dominion5254 adds the line

COPY ./.git/modules/clboss /tmp/.git/modules/clboss to the dockerfile before the clboss make happens

I agree that this is the best way to solve the problem, as the way it was previously the commit_hash.h was just a dummy file that is not useful

# cat commit_hash.h 
#pragma once
#define GIT_COMMIT_HASH ""
#define GIT_DESCRIBE ""

Now with this code (and the modification to the downstream dockerfile) the new commit_hash.h is:

# cat commit_hash.h 
#pragma once
#define GIT_COMMIT_HASH "88ba9f22feca12073685979ccbca51093d47f9a9"
#define GIT_DESCRIBE "v0.14.0-3-g88ba9f2-dirty"

I will update the PR shortly.

@chrisguida chrisguida force-pushed the build-as-submodule branch 2 times, most recently from 70eb725 to 339cc50 Compare October 26, 2024 01:35
@Dominion5254
Copy link

@chrisguida using your build-as-submodule branch and adding the suggested line - COPY ./.git/modules/clboss /tmp/.git/modules/clboss to the cln-startos Dockerfile does seem to generate the expected commit_hash.h file.

#19 [clboss 11/13] RUN cat /tmp/clboss/commit_hash.h
#19 0.039 #pragma once
#19 0.039 #define GIT_COMMIT_HASH "339cc5065bb8c167f5dcb78c8b6d87f6670bf54d"
#19 0.039 #define GIT_DESCRIBE "v0.14.0-4-g339cc50"

@ksedgwic if you are good with Chris's PR, would this change be worth a point release of clboss, or would you just include this in the next planned release? I don't really have a preference as I have a working build for cln-startos in either case.

@chrisguida
Copy link
Contributor Author

chrisguida commented Oct 28, 2024

I think a point release could be worth it (or you could just build cln-startos from clboss master), because it looks like, even though the build is succeeding when you call generate_hash.sh manually, the hash is actually not being populated. Fixing this would allow start9 users to provide much more useful data upstream to the clboss team, as they would be able to tell us the exact commit hash they're on.

That being said, assuming that the cln-startos release is locked to a particular commit of clboss, this shouldn't really matter as long as users report which version of cln-startos they're using.

@ksedgwic ksedgwic merged commit 965e131 into ZmnSCPxj:master Oct 28, 2024
1 check passed
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.

[Bug]: 0.14.0 Build Error
3 participants