Add initial gomobile support #1164

divan · 2018-08-22T12:47:04Z

This PR adds initial "drop-in" gomobile support for status-go.
"Drop-in" means it's supposed to be used as a replacement to the current xgo-based lib package. I.e. generated .aar/.framework files should be absolutely the same and require no modification from status-react in order to work.

Important changes:

Add gomobile installation to the make setup
Replace statusgo-ios and statusgo-android to use gomobile instead of xgo
Remove Jenkinsfile-manual

Closes #680

divan · 2018-08-22T14:19:37Z

Codeclimate doesn't like duplication in lib and mobile packages (but it's needed for transition). Travis seems to fail due to flack test TestPeerPoolSimulationSuite/TestMailServerPeersDiscovery.

adambabik · 2018-08-27T07:09:27Z

Makefile

@@ -171,7 +164,11 @@ xgo:
 	docker pull $(XGOIMAGE)
 	go get github.com/karalabe/xgo

-setup: dep-install lint-install mock-install ##@other Prepare project for first build
+gomobile:


gomobile-install?

make sense, thanks!

adambabik · 2018-08-27T07:14:03Z

mobile/response.go

@@ -0,0 +1,64 @@
+package status


We can either move it to types.go or change it to jsonrpc_response.go.

It's an issue that we have so many response types but this one was introduced for bindings that are meant to replace JSON-RPC calls. In status-react, commands like eth_sendTransaction are intercepted and a binding is called instead. Thanks to this type of response, the response can be easily returned without further processing.

dshulyak

comments are mostly irrelevant, but what is the thread situation in code generated by gomobile? as I remember we are using a thread pool in objective-c and java, and application seems to depend on it

dshulyak · 2018-08-27T07:21:38Z

mobile/status.go

+		return makeJSONResponse(err)
+	}
+
+	api.RunAsync(func() error { return statusBackend.StartNode(config) })


not really related to this PR. but it seems that RunAsync is totally unnecessary, every call is executed in its own thread. so, for example, StartNode can return an actual error, instead of using signals to report that error. and that means - part of the signals are not useful at all. and the other part of signals can be a simple a subscription over RPC.

dshulyak · 2018-08-27T07:22:35Z

mobile/status.go

+var statusBackend = api.NewStatusBackend()
+
+// All general log messages in this package should be routed through this logger.
+var logger = log.New("package", "status-go/lib")


do you want to keep the name of logger (lib part)?

dshulyak · 2018-08-27T07:25:27Z

mobile/status.go

+	)
+	errString := ""
+
+	defer func() {


this defer usage is so confusing. it definitely should be a simple function that accepts error and returns string

divan · 2018-09-03T10:28:10Z

@dshulyak answering your comments here in bulk: this PR is "drop-in" replacement, I want to change as little as possible in the original implementation. So I guess the followup PR will be a rethinking the interface between status-go and react with gomobile in mind, and most of your comments will be for sure resolved there.

I'm not quite sure I understood the thread pool issue. Can you elaborate on it?

dshulyak · 2018-09-04T08:23:48Z

@divan so both versions of the app are running on a thread pool. but recently there were performance issues on android. after investigation it turned out that pool size was too small. i think it was set to the number of cpu cores (or a double of that). raising number of threads helped. i don't know details of those performance problems, but in current implementation there must be a peer pool in andoid/ios bindings. otherwise app won't work. cc @pombeirp in case he wants to provide more details

pedropombeiro · 2018-09-21T09:27:43Z

From a quick test, it seems that there are some open questions:

make statusgo-ios needs XCode to run, so we'd need to adapt the Makefiles to run each build on the appropriate host;
make statusgo-android needs some environment variables, so it can find things like the NDK. We need to think what the best way is to go around that. Right now the documentation doesn't reflect that (btw the documentation has moved to docs.status.im in develop).

xgo is not removed yet, because it's probably still needed for status-desktop (to confirm)

If we collectively decide that it is not absolutely required to be able to cross-compile iOS/MacOS libraries, then I'd definitely get rid of xgo, as otherwise I don't see much advantage in bringing gomobile to the equation.

ghost · 2018-09-21T09:27:48Z

Pull Request Checklist

Have you updated the documentation, if impacted (e.g. docs.status.im)?

divan · 2018-09-21T13:19:35Z

Thanks for feedback @pombeirp. So let's breakdown the problems we have:

Unclear if it's possible to make an iOS gomobile build on Linux machine (Jenkins/CI)
Lack of documentation of what to do when Android NDK is not installed

Is that correct?

Regarding the Android NDK: I had an assumption that gomobile init does all the needed checks and validations, so no further env variables needed to be specified, but I have to test this assumption.

pedropombeiro · 2018-09-21T13:55:58Z

Yeah, that's correct. Although I'm quite sure that XCode doesn't run on anything but a Mac. When you mentioned the PR was ready for testing on the react side (pending being able to build status-react), I thought you had already tested the two types of builds to confirm that the libraries were generated correctly, but if I understand correctly, that still remains to be done, right?

pedropombeiro · 2018-09-25T09:14:45Z

As future context: I'm currently working on another PR which is meant to provide status-go static library artifacts to Status Desktop builds. I'm running into issues consuming the cross-compiled OSX library, so I'm starting to think that with the move to gomobile, we'll already be tied to OSX hosts anyway, so might as well stop relying on xgo altogether and ensure that we build OSX artifacts on OSX hosts. cc @jakubgs

dmitryn · 2018-11-06T10:56:23Z

I'm trying to make manual build with gomobile branch to integrate it into status-react, but CI fails with:
https://ci.status.im/job/status-go/job/status-go-manual/544/console

[status-go] Running shell script
+ make ci
lint
level=warning msg="[runner/megacheck] Can't run megacheck because of compilation errors in packages [github.com/status-im/status-go/mobile]: mobile/status.go:67: LoadNodeConfig not declared by package params and 6 more errors: run `golangci-lint run --no-config --disable-all -E typecheck` to see all errors"
mobile/status.go:67:19: LoadNodeConfig not declared by package params (typecheck)
	_, err := params.LoadNodeConfig(configJSON)
	                 ^
mobile/status.go:43:24: LoadNodeConfig not declared by package params (typecheck)
	config, err := params.LoadNodeConfig(configJSON)
	                      ^
mobile/status.go:28:47: cannot convert "" (untyped string constant) to uint64 (typecheck)
	config, err := params.NewNodeConfig(datadir, "", params.FleetBeta, uint64(networkID))
	                                             ^
Makefile:260: recipe for target 'lint' failed
make: *** [lint] Error 1
[Pipeline] }
[Pipeline] // dir
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
ERROR: script returned exit code 2
Finished: FAILURE

@divan is there any instructions on how to build it?

status-go » status-go-manual #544 Console [Jenkins]

adambabik · 2018-11-06T14:25:40Z

This branch is just outdated as some packages in status-go changed.

divan · 2018-11-06T18:07:07Z

I pushed the commit with rebase, please try now.

dmitryn · 2018-11-07T16:07:23Z

Failing:

[status-go-manual] Running shell script
+ go get github.com/status-im/xgo
[Pipeline] parallel
[Pipeline] [statusgo-android] { (Branch: statusgo-android)
[Pipeline] [statusgo-ios-simulator] { (Branch: statusgo-ios-simulator)
[Pipeline] [statusgo-android] dir
[statusgo-android] Running in /home/jenkins/workspace/status-go/status-go-manual/src/github.com/status-im/status-go
[Pipeline] [statusgo-android] {
[Pipeline] [statusgo-ios-simulator] dir
[statusgo-ios-simulator] Running in /home/jenkins/workspace/status-go/status-go-manual/src/github.com/status-im/status-go
[Pipeline] [statusgo-ios-simulator] {
[Pipeline] [statusgo-android] sh
[statusgo-android] [status-go] Running shell script
[statusgo-android] + make statusgo-android
[Pipeline] [statusgo-ios-simulator] sh
[statusgo-android] Installing gomobile...
[statusgo-ios-simulator] [status-go] Running shell script
[statusgo-ios-simulator] + make statusgo-ios-simulator
[statusgo-ios-simulator] make: *** No rule to make target 'statusgo-ios-simulator'.  Stop.
[Pipeline] [statusgo-ios-simulator] }
[Pipeline] [statusgo-ios-simulator] // dir
[Pipeline] [statusgo-ios-simulator] }
[statusgo-ios-simulator] Failed in branch statusgo-ios-simulator
[statusgo-android] Building status-go for Android...
[statusgo-android] gomobile: no Android NDK path is set. Please run gomobile init with the ndk-bundle installed through the Android SDK manager or with the -ndk flag set.
[statusgo-android] Makefile:118: recipe for target 'statusgo-android' failed
[statusgo-android] make: *** [statusgo-android] Error 1
[Pipeline] [statusgo-android] }
[Pipeline] [statusgo-android] // dir
[Pipeline] [statusgo-android] }
[statusgo-android] Failed in branch statusgo-android
[Pipeline] // parallel
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
ERROR: script returned exit code 2
Finished: FAILURE

https://ci.status.im/job/status-go/job/status-go-manual/545/console

status-go » status-go-manual #545 Console [Jenkins]

divan · 2018-11-08T18:34:03Z

@dmitryn hmm, so Makefile should have gomobile init command to prepare CI environment. Can you manually put this command to pipeline before Makefile is executing?

dmitryn · 2018-11-09T12:59:05Z

@divan I've added gomobile init before make statusgo-android and make statusgo-ios-simulator, but it didn't help

+ go get github.com/status-im/xgo
[Pipeline] parallel
[Pipeline] [statusgo-android] { (Branch: statusgo-android)
[Pipeline] [statusgo-ios-simulator] { (Branch: statusgo-ios-simulator)
[Pipeline] [statusgo-android] dir
[statusgo-android] Running in /home/jenkins/workspace/status-go/status-go-manual/src/github.com/status-im/status-go
[Pipeline] [statusgo-android] {
[Pipeline] [statusgo-ios-simulator] dir
[statusgo-ios-simulator] Running in /home/jenkins/workspace/status-go/status-go-manual/src/github.com/status-im/status-go
[Pipeline] [statusgo-ios-simulator] {
[Pipeline] [statusgo-android] sh
[statusgo-android] [status-go] Running shell script
[statusgo-android] + gomobile init
[Pipeline] [statusgo-ios-simulator] sh
[statusgo-ios-simulator] [status-go] Running shell script
[statusgo-ios-simulator] + sh gomobile init
[statusgo-ios-simulator] sh: 0: Can't open gomobile init
[Pipeline] [statusgo-ios-simulator] }
[Pipeline] [statusgo-ios-simulator] // dir
[Pipeline] [statusgo-ios-simulator] }
[statusgo-ios-simulator] Failed in branch statusgo-ios-simulator
[Pipeline] [statusgo-android] sh
[statusgo-android] [status-go] Running shell script
[statusgo-android] + make statusgo-android
[statusgo-android] Installing gomobile...
[statusgo-android] Building status-go for Android...
[statusgo-android] gomobile: no Android NDK path is set. Please run gomobile init with the ndk-bundle installed through the Android SDK manager or with the -ndk flag set.
[statusgo-android] Makefile:118: recipe for target 'statusgo-android' failed
[statusgo-android] make: *** [statusgo-android] Error 1
[Pipeline] [statusgo-android] }
[Pipeline] [statusgo-android] // dir
[Pipeline] [statusgo-android] }
[statusgo-android] Failed in branch statusgo-android
[Pipeline] // parallel
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
ERROR: script returned exit code 127
Finished: FAILURE

divan · 2018-11-09T13:04:42Z

@dmitryn that's because sh gomobile init failed – sh seems to expect sh script as an input, not the command itself.

ghost · 2018-11-23T13:11:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Signed-off-by: Jakub Sokołowski <jakub@status.im>

mandrigin · 2019-02-01T16:41:12Z

android: works

mandrigin · 2019-02-01T16:49:08Z

iOS: works.

mandrigin · 2019-02-01T17:03:45Z

thanks @divan for starting it!
@adambabik for wrapping it up!
@jakubgs for fighting dear god scary nightmares of Jenkins!

divan added 2 commits August 22, 2018 12:09

Add drop-in lib package for usage with gomobile

7a05ce6

Update Makefile to use gomobile

94b9880

divan force-pushed the gomobile branch from a7e90c0 to ff60900 Compare August 22, 2018 12:50

Add README to mobile

041990b

divan force-pushed the gomobile branch from ff60900 to 041990b Compare August 22, 2018 12:52

adambabik reviewed Aug 27, 2018

View reviewed changes

dshulyak approved these changes Aug 27, 2018

View reviewed changes

divan added 2 commits October 27, 2018 17:15

Merge branch 'develop' into gomobile

f2610b5

Fix library API

6fa22db

ghost added the stale label Nov 23, 2018

adambabik added 4 commits November 26, 2018 10:16

clean up Makefile

b178f25

Merge branch 'develop' into gomobile

0718c78

update missing exports

606ddfd

clean up

a628bd2

jakubgs force-pushed the gomobile branch 4 times, most recently from d7b5267 to 49b3335 Compare February 1, 2019 09:42

Fix how we stash Statusgo.framework

63ad571

Signed-off-by: Jakub Sokołowski <jakub@status.im>

jakubgs force-pushed the gomobile branch from 49b3335 to 63ad571 Compare February 1, 2019 09:59

mandrigin and others added 18 commits February 1, 2019 11:20

Tweak "Release" phase.

44e373a

fix syntax error in Jenkinsfile (eeh)

09c9025

one more syntax fixup

3553a48

:(

0e0f7f4

add debug output

bd32400

add cleanup code

0e2aeba

fix dir path

820e398

make Android stash look more like iOS

c787063

Clean-up after yourself.

bd8285d

more debug output

ae68018

even more debug info

a0141f6

add more debug info

e1fb48e

try not to clean-up files after stashing

a88795b

Merge branch 'develop' into gomobile

3395494

Add UpdateMailservers to gomobile exports.

4a5ef3a

split builds into separate Jenkinsfiles

1fa9103

Signed-off-by: Jakub Sokołowski <jakub@status.im>

Merge remote-tracking branch 'origin/gomobile-split' into gomobile

5634b92

bump version to .1

47557cd

jakubgs approved these changes Feb 1, 2019

View reviewed changes

mandrigin merged commit 4f3f5ee into develop Feb 1, 2019

delete-merged-branch bot deleted the gomobile branch February 1, 2019 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial gomobile support #1164

Add initial gomobile support #1164

divan commented Aug 22, 2018 •

edited by adambabik

Loading

divan commented Aug 22, 2018

adambabik Aug 27, 2018

divan Sep 3, 2018

adambabik Aug 27, 2018

dshulyak left a comment

dshulyak Aug 27, 2018

dshulyak Aug 27, 2018

dshulyak Aug 27, 2018

divan commented Sep 3, 2018

dshulyak commented Sep 4, 2018

pedropombeiro commented Sep 21, 2018 •

edited

Loading

ghost commented Sep 21, 2018

divan commented Sep 21, 2018

pedropombeiro commented Sep 21, 2018

pedropombeiro commented Sep 25, 2018 •

edited

Loading

dmitryn commented Nov 6, 2018 •

edited by ghost

Loading

adambabik commented Nov 6, 2018

divan commented Nov 6, 2018

dmitryn commented Nov 7, 2018 •

edited by ghost

Loading

divan commented Nov 8, 2018

dmitryn commented Nov 9, 2018

divan commented Nov 9, 2018

ghost commented Nov 23, 2018

mandrigin commented Feb 1, 2019

mandrigin commented Feb 1, 2019

mandrigin commented Feb 1, 2019

Add initial gomobile support #1164

Add initial gomobile support #1164

Conversation

divan commented Aug 22, 2018 • edited by adambabik Loading

divan commented Aug 22, 2018

adambabik Aug 27, 2018

Choose a reason for hiding this comment

divan Sep 3, 2018

Choose a reason for hiding this comment

adambabik Aug 27, 2018

Choose a reason for hiding this comment

dshulyak left a comment

Choose a reason for hiding this comment

dshulyak Aug 27, 2018

Choose a reason for hiding this comment

dshulyak Aug 27, 2018

Choose a reason for hiding this comment

dshulyak Aug 27, 2018

Choose a reason for hiding this comment

divan commented Sep 3, 2018

dshulyak commented Sep 4, 2018

pedropombeiro commented Sep 21, 2018 • edited Loading

ghost commented Sep 21, 2018

Pull Request Checklist

divan commented Sep 21, 2018

pedropombeiro commented Sep 21, 2018

pedropombeiro commented Sep 25, 2018 • edited Loading

dmitryn commented Nov 6, 2018 • edited by ghost Loading

adambabik commented Nov 6, 2018

divan commented Nov 6, 2018

dmitryn commented Nov 7, 2018 • edited by ghost Loading

divan commented Nov 8, 2018

dmitryn commented Nov 9, 2018

divan commented Nov 9, 2018

ghost commented Nov 23, 2018

mandrigin commented Feb 1, 2019

mandrigin commented Feb 1, 2019

mandrigin commented Feb 1, 2019

divan commented Aug 22, 2018 •

edited by adambabik

Loading

pedropombeiro commented Sep 21, 2018 •

edited

Loading

pedropombeiro commented Sep 25, 2018 •

edited

Loading

dmitryn commented Nov 6, 2018 •

edited by ghost

Loading

dmitryn commented Nov 7, 2018 •

edited by ghost

Loading