feat: libwaku dll for status go #3460
Conversation
|
You can find the image built from this PR at Built from 26fdbbf |
Ivansete-status
left a comment
There was a problem hiding this comment.
Thanks for it!
Nevertheless, I think we need to double-check it :)
When trying to build on Ubuntu, I'm getting the following:
12:10:28 {libwaku_dll} ~/workspace/status/nwaku$
$ [Nimbus env]$ make libwaku -j8
Warning: to ensure you are building in a supported repo state, please always run "make update" after "git pull"
(it looks like you've forgotten to do this).
This also applies whenever you switch to a new branch or commit, e.g.: whenever you run "git checkout ...".
bash: -c: line 1: syntax error near unexpected token `Linux,Windows'
bash: -c: line 1: `ifeq (Linux,Windows)'
make: *** [Makefile:406: libwaku] Error 2Cheers
scripts/build_wakunode_windows.sh
Outdated
| execute_command "make wakunode2 LOG_LEVEL=DEBUG V=1 -j8" | ||
| execute_command "make wakunode2 LOG_LEVEL=DEBUG V=1 -j" | ||
|
|
||
| echo "9. -.-.-.- Building libwaku -.-.-.- " |
There was a problem hiding this comment.
| echo "9. -.-.-.- Building libwaku -.-.-.- " | |
| echo "10. -.-.-.- Building libwaku -.-.-.- " |
| exec "nim c" & " --out:build/" & name & | ||
| ".so --threads:on --app:lib --opt:size --noMain --mm:refc --header -d:metrics --nimMainPrefix:libwaku --skipParentCfg:on -d:discv5_protocol_id=d5waku " & | ||
| extra_params & " " & srcDir & name & ".nim" | ||
| when defined(windows): |
There was a problem hiding this comment.
It seems the unix-based OSs are forgotten somehow
waku.nimble
Outdated
| --warning:Deprecated:off \ | ||
| --warning:UnusedImport:on \ | ||
| -d:chronicles_log_level=TRACE """, | ||
| """-d:chronicles_line_numbers -d:chronicles_runtime_filtering=on -d:chronicles_sinks="textlines,json" -d:chronicles_default_output_device=Dynamic -d:chronicles_disabled_topics="eth,dnsdisc.client" --warning:Deprecated:off --warning:UnusedImport:on -d:chronicles_log_level=TRACE """, |
There was a problem hiding this comment.
Let's keep it vertical :)
Easier to read and easier to see diffs on each line in future PRs
Also applicable to the next change
There was a problem hiding this comment.
Thanks for comment @Ivansete-status, I’ve addressed all your comments.
There was a problem hiding this comment.
I’m getting a unrealistic error with vertical param, I can’t debug yet, but I’m working on it.
There was a problem hiding this comment.
- @darshankabariya Isn't that because of the multiline string literal by
"""- triple quote?
There was a problem hiding this comment.
Yeah, it was a spacing issue. I’ve fixed it using string concatenation.
Makefile
Outdated
| else ifeq ($(detected_OS),Windows) | ||
| echo -e $(BUILD_MSG) "build/$@.dll" && $(ENV_SCRIPT) nim libwakuDLL $(NIM_PARAMS) waku.nims |
There was a problem hiding this comment.
I would rather make separation by target platfom in one place of possible.
For me it sounds that shouldn't be different command in the makefile to build dyn lib in different platforms.
I would move it to nimble better.
But maybe it is just my taste :-)
There was a problem hiding this comment.
Got it. I wasn’t sure it would work without OS checks since we need to echo .dll or .so. But your idea helped — we can drop libwakuDLL, libwakuDynamic can handle both libs.
waku.nimble
Outdated
| elif `type` == "dll": | ||
| exec "nim c" & " --out:build/" & name & | ||
| ".dll --nimcache:build/nimcache --threads:on --app:lib --opt:size --noMain --mm:refc --header -d:metrics --nimMainPrefix:libwaku -d:discv5_protocol_id=d5waku " & |
There was a problem hiding this comment.
nimscript has support for properly add extension to target filename:
toDll and toExe
https://github.com/nim-lang/Nim/blob/f7145dd26efeeeb6eeae6fff649db244d81b212d/lib/system/nimscript.nim#L147
Using these can simplify nimble code imho.
But beware toDll will result lib+name+ext... so maybe we need to replicate it for our purpose.
There was a problem hiding this comment.
Thanks, @NagyZoltanPeter — I’ve addressed this suggestion.
41b2f73 to
5737fa8
Compare
916e2eb to
6c8630a
Compare
| var lib_name = toDll("libwaku") | ||
| when defined(windows): | ||
| exec "nim c" & " --out:build/" & lib_name & | ||
| ".so --threads:on --app:lib --opt:size --noMain --mm:refc --header -d:metrics --nimMainPrefix:libwaku --skipParentCfg:off -d:discv5_protocol_id=d5waku " & | ||
| extra_params & " " & srcDir & name & ".nim" | ||
| else: | ||
| exec "nim c" & " --out:build/" & lib_name & | ||
| ".so --threads:on --app:lib --opt:size --noMain --mm:refc --header -d:metrics --nimMainPrefix:libwaku --skipParentCfg:on -d:discv5_protocol_id=d5waku " & | ||
| extra_params & " " & srcDir & name & ".nim" |
There was a problem hiding this comment.
warn: Isn't toDll("libwaku") will return liblibwaku.so or liblibwaku.dll?
Well also if we still need to branch by platform due to --skipParentCfg... I'm questioning my original reasoning here. But it's hard to recognise at first sight. Btw, why is this distinction?
There was a problem hiding this comment.
Thanks for pointing out the distinction with --skipParentCfg. On Windows, building the binary without external requirements is a bit tricky. Once we fully integrate windows support into the make command, we can revisit and properly address this. For now, I’ve removed --skipParentCfg specifically for the Windows build. cc (@NagyZoltanPeter)
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
Thank you!!! Awesome!
b82ea5c to
1041224
Compare
gabrielmer
left a comment
There was a problem hiding this comment.
LGTM, thanks so much!
Added support for building dynamic libraries (*.dll) on Windows systems.
Now, both libwaku.dll and wakunode2.exe can be generated using ./script/build_windows.sh.
Also added CI checks to verify the successful build of libwaku.dll.
@gabrielmer