-
Notifications
You must be signed in to change notification settings - Fork 91
chore: status-go with nim-sds #19402
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
base: master
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (223)
|
7f50f38 to
773af07
Compare
ci/Jenkinsfile.linux
Outdated
| /* nwaku source directory */ | ||
| NWAKU_SOURCE_DIR = "${env.WORKSPACE_TMP}/nwaku" | ||
| /* nim-sds source directory */ | ||
| NIM_SDS_SOURCE_DIR = "${env.WORKSPACE}/nim-sds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WORKSPACE_TMP must be used for nwaku and nim-sds.
| NIM_SDS_SOURCE_DIR = "${env.WORKSPACE}/nim-sds" | |
| NIM_SDS_SOURCE_DIR = "${env.WORKSPACE_TMP}/nim-sds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all CI jobs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we must set NIM_SDS_SOURCE_DIR in all CI jobs.
So we should also set it for android and windows.
mobile/Makefile
Outdated
| IPHONE_SDK="$(IPHONE_SDK)" \ | ||
| IOS_TARGET="$(IOS_TARGET)" \ | ||
| SHELL=/bin/sh | ||
| @cp $(WORKSPACE)/nim-sds/build/libsds$(LIB_EXT) $(LIB_PATH)/libsds$(LIB_EXT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the WORKSPACE variable you're referencing? The one defined in Jenkins? And how to build it locally then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't reference pure Jenkins variables here. Instead, we should use NIM_SDS_LIB_DIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we hard-code the path as in this example?
@cp ../vendor/status-go/build/bin/libstatus$(LIB_EXT) $(LIB_PATH)/libstatus$(LIB_EXT)
mobile/Makefile
Outdated
| HOST_OS="$(HOST_OS)" \ | ||
| GO_GENERATE_CMD="go generate" \ | ||
| SHELL=/bin/sh | ||
| @cp ../vendor/nim-sds/build/libsds$(LIB_EXT) $(LIB_PATH)/libsds$(LIB_EXT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, use NIM_SDS_LIB_DIR
Makefile
Outdated
| STATUSGO_MAKE_PARAMS += NIM_SDS_HEADER_PATH="$(LIBSDS_LIBDIR)" | ||
| STATUSGO_MAKE_PARAMS += NIM_SDS_LIB_PATH="$(GIT_ROOT)/vendor/nim-sds/build/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, status-go should be provided with NIM_SDS_SOURCE_DIR, not binary paths.
Makefile
Outdated
| NIM_EXTRA_PARAMS += --passL:"-L$(LIBWAKU_LIBDIR)" --passL:"-lwaku" | ||
| endif | ||
|
|
||
| NIM_SDS_SOURCE_DIR ?= vendor/nim-sds/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default should be outside the repository. Same as we do for nwaku
| NIM_SDS_SOURCE_DIR ?= vendor/nim-sds/ | |
| NIM_SDS_SOURCE_DIR ?= $(GIT_ROOT)/../nim-sds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thank you 👍
e2e tests are probably failing because you're upgrading status-go, and I still didn't merge my fix #19283. Will do.
(can't approve as it's my own pr)
| pkg-macos: check-pkg-target-macos $(STATUS_CLIENT_DMG) | ||
| clean-libsds-cache: | ||
| @echo "Cleaning libsds_d from cache..." | ||
| rm -rf ~/.cache/nim/libsds_d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seem to be very specific. What's this about? Will it work on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to avoid compilation issues between macos and iOS. i.e., error such as:
ld: building for 'macOS', but linking in object file
(/Users/ivan/.cache/nim/libsds_d/@m..@svendor@snimbus-build-system@svendor@sNim@slib@ssystem@sexceptions.nim.c.o)
built for 'iOS-simulator'
727ae41 to
37afaef
Compare
No description provided.