Add build tags to signal package #1119

divan · 2018-07-26T17:38:18Z

This PR introduces build tags (or build constraints) to the signal package. The code is this package differs for different platforms and build modes, namely:

iOS build as a library (require CGo)
Android build as a library (require CGo)
Desktop build as a library (require CGo)
normal build of statusd or running local tests (not requiring CGo)

It remove CGo dependency on signal package for statusd and tests. (Also, part of the memleak mitigation (
#1067 (comment)).

Also, it would be nice to have non-Cgo variant a default (easier with go get, tests, vendoring, etc.)

a new build tag introduced - library. No need to use it manually, it's added to Makefile for commands statusgo-library, statusgo-ios and statusgo-android.
Android and iOS specific C code moved to signals_android.c and signals_darwin_arm.c respectively
Library/CGo-specific code moved to signals_library.go (and signals_darwin.go because of two iOS specific CGo imports that need to be in that Go file for CGo magic to work)
non-library (default) signals implementation that doesn't link with CGo is moved to signals.go

More info on build tags: https://golang.org/pkg/go/build/#hdr-Build_Constraints and https://dave.cheney.net/2013/10/12/how-to-use-conditional-compilation-with-the-go-build-tool

As there is a build code changes involved and file layout changed, I would be pleasantly surprised if CI, tests and cross build with status-react and status-desktop will Go smoothly :)

Closes #1115

build - The Go Programming Language

How to use conditional compilation with the go build tool | Dave Cheney

adambabik

Great effort! It's much better than C macros. I am a little bit confused by build tags which seems inconsistent/missing in some places.

adambabik · 2018-07-28T12:08:34Z

Makefile

 	./_assets/patches/patcher -b . -p geth-xgo -r
 	@echo "iOS framework cross compilation done."

 statusgo-library: ##@cross-compile Build status-go as static library for current platform
 	@echo "Building static library..."
-	go build -buildmode=c-archive -o $(GOBIN)/libstatus.a ./lib
+	go build -buildmode=c-archive -tags library -o $(GOBIN)/libstatus.a ./lib


Shouldn't this line also read as -tags library $(BUILD_TAGS)? It might have been missed before.

@adambabik so I anticipated removal of BUILD_TAGS in this PR. Does it make sense to remove it after #1121? Or we still want/need to define BUILD_TAGS from the environment?

I don't know of any use case for BUILD_TAGS now so we can remove all references.

adambabik · 2018-07-28T12:11:18Z

signal/signals.go

-	str := C.CString(string(data))
-	C.StatusServiceSignalEvent(str)
-	C.free(unsafe.Pointer(str))
+	notificationHandlerMutex.Lock()


RLock() if we have sync.RWMutex. Regular mutex should also be fine.

Right, good spot!

adambabik · 2018-07-28T12:13:40Z

signal/signals_android.c

@@ -0,0 +1,153 @@
+// ======================================================================================


Does it miss // +build library,android?

Uhm, I don't think so, but it might be worth adding. The main issue here is that both build tags and file naming conventions are kinda redundant, and I don't want to bring confusion by using both. So, here I used _android.c name and it seems to work properly because CGo now is enabled (and this means that C files are even considered by compiler to look into) only on library builds (and that, in its turn enforced by build tags in .go files).

So it's a bit complicated, but I'm going to refactor signals anyway, so at least this combination works for now. (I need to address tests fails on CI though, maybe they're related)

So does it mean that naming a file signals_android.c will compile it only for android environment?

@adambabik yes! I didn't find any mention of it in docs, but empirically that's the way how it seemed to work (I spent almost a day, trying out those things and waiting for xgo builds)

Thanks, now it makes more sense!

adambabik · 2018-07-28T12:14:19Z

signal/signals_darwin_arm.c

@@ -0,0 +1,49 @@
+// ======================================================================================


Similarly here // +build library,darwin,arm?

adambabik · 2018-07-28T12:14:43Z

signal/signals.c

-	int result = JNI_VERSION_1_6;
-
-	gJavaVM = vm;
+// +build library,!android,!arm


!darwin missing?

No, darwin is a valid architecture name for MacOS X.

adambabik · 2018-07-28T12:15:16Z

signal/signals_library.go

@@ -0,0 +1,101 @@
+// +build library,!darwin


!android?

So signals_library.go is for both Desktop and Android builds (their C files are identical). I was thinking to make it more explicit and just copy code, but then decided to do this way. Anyway, going to refactor it asap.

It's interesting how only three dimensions of build options can create so much confusion :)

…n thousands of line of CI test output

divan · 2018-07-29T15:27:07Z

Codeclimate doesn't understand Go build tags.

divan · 2018-07-29T15:29:04Z

The CI was falling due to current signals design. Seems like signals handler was set from two different places during the test, and so test was filing. After introducing debug output, it worked fine, because of tiny delay. Which, again, shows that signals package begs for refactoring.

This reverts commit 27e432a.

divan added 5 commits July 26, 2018 19:07

Add 'library' tag to Makefile for all library builds

fb3e673

Add build tags for library/non-library, and arch-specific builds

2db308b

Remove verbose output option from ios build

eaf6e56

Add handlers code back to make tests work before refactoring

0661cbf

Add missing stub

6e173bc

divan mentioned this pull request Jul 27, 2018

Improve generating Docker image tags #1121

Closed

adambabik reviewed Jul 28, 2018

View reviewed changes

divan added 3 commits July 29, 2018 14:29

Address review comments

555e8a1

Reduce test verbosity because I simply cannot find where failure is i…

a674db4

…n thousands of line of CI test output

Add debug output for tests

1dd2a10

divan merged commit 27e432a into develop Aug 7, 2018

adambabik added a commit that referenced this pull request Aug 9, 2018

Revert "Add build tags to signal package (#1119)"

b80ed8c

This reverts commit 27e432a.

divan mentioned this pull request Aug 9, 2018

Fix signal package build for iOS #1146

Closed

adambabik added a commit that referenced this pull request Aug 10, 2018

Revert "Add build tags to signal package (#1119)"

d7d9907

adambabik added a commit that referenced this pull request Aug 10, 2018

Revert "Add build tags to signal package (#1119)" (#1148)

f26e849

This reverts commit 27e432a.

pedropombeiro deleted the signal_tags branch August 15, 2018 13:56

		@@ -0,0 +1,153 @@
		// ======================================================================================

		@@ -0,0 +1,49 @@
		// ======================================================================================

Add build tags to signal package #1119

Add build tags to signal package #1119

Uh oh!

Conversation

divan commented Jul 26, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adambabik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divan Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divan commented Jul 29, 2018

Uh oh!

divan commented Jul 29, 2018

Uh oh!

Uh oh!

divan commented Jul 26, 2018 •

edited by ghost

Loading

divan Jul 29, 2018 •

edited

Loading