waku-org / nwaku

Waku node and protocol.
Other
184 stars 47 forks source link

bug: vendor/zerokit/target/release/librln.so #1372

Closed fryorcraken closed 1 year ago

fryorcraken commented 1 year ago

Problem

When building nwaku master HEAD in GitHub action CI to spot any incoming incompatibility:

PROC=$(nproc --all 2>/dev/null || echo 2)
make -j$PROC update
NIMFLAGS="-d:chronicles_colors=off -d:chronicles_sinks=textlines -d:chronicles_log_level=TRACE" make -j$PROC wakunode2

Once compiled, getting following error:

could not load: vendor/zerokit/target/release/librln.so

See CI action: https://github.com/waku-org/js-waku/actions/runs/3442478506/jobs/5743098617

Impact

Not great :)

Reproduce

See https://github.com/waku-org/js-waku/pull/1017

Screenshots/logs

See CI action: https://github.com/waku-org/js-waku/actions/runs/3442478506/jobs/5743098617

nwaku version/commit hash

master HEAD

fryorcraken commented 1 year ago

Also, do I still need to set the NIMFLAGS to get trace logs or is that deprecated?

jm-clius commented 1 year ago

cc @s1fr0 @rymnc (haven't reproduced myself, but please take a look).

jm-clius commented 1 year ago

Also, do I still need to set the NIMFLAGS to get trace logs or is that deprecated?

Deprecated. You can now use --log-level runtime to choose your desired log level.

rymnc commented 1 year ago

Will try to reproduce today.

s1fr0 commented 1 year ago

Most likely is a path problem: I see from logs that librln is compiled at /home/runner/work/js-waku/js-waku/nwaku/vendor/zerokit/rln and is trying to load it from /vendor/zerokit/rln instead https://github.com/status-im/nwaku/tree/master/waku/v2/protocol/waku_rln_relay#L12.

It is the first time that this bug happen? We never actually changed that path (modulo kilic/zerokit)

EDIT: another quite reasonable explanation is that we removed a necessary when defined(rln) flag in the kilic lib removal.

jm-clius commented 1 year ago

another quite reasonable explanation is that we removed a necessary when defined(rln) flag in the kilic lib removal.

Most likely explanation IMO. The path shouldn't matter if RLN is not defined.

rymnc commented 1 year ago

@fryorcraken, can you update your PR (https://github.com/waku-org/js-waku/pull/1017) to use https://github.com/status-im/nwaku/pull/1373 as the origin?

s1fr0 commented 1 year ago

I believe the changes in this branch would suffice too, in case we want to be a bit more surgical on the changes nwaku-side.

I was trying to trigger CI tests in a local fork of js-waku, but failed while dealing with nwaku branches/workflows.

fryorcraken commented 1 year ago

@fryorcraken, can you update your PR (waku-org/js-waku#1017) to use #1373 as the origin?

Finally got it to happen:

Run uname -a
  uname -a
  cd nwaku/buildabove
  ./wakunode2 --help
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    NWAKU_VERSION: v0.11
    NODE_JS: 16
    DEBUG: waku*
    npm_config_cache: /home/runner/.npm
Linux fv-az224-67 5.15.0-1022-azure #27~20.04.1-Ubuntu SMP Mon Oct 17 02:03:50 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
/home/runner/work/_temp/13525db4-816b-4f32-9b6c-139625b243db.sh: line 2: cd: nwaku/buildabove: No such file or directory
Error: Process completed with exit code 1.

https://github.com/waku-org/js-waku/actions/runs/3457913371/jobs/5771813882

Not sure if nwaku bug or issue with the checkout. let me know.

rymnc commented 1 year ago

Path should be nwaku/build not nwaku/buildabove 🤔

fryorcraken commented 1 year ago

Path should be nwaku/build not nwaku/buildabove thinking

My brain did not even register. That's what happened when you multitask.. ok I pushed again. :crossed_fingers:

rymnc commented 1 year ago

Addressed in #1373, @fryorcraken please close this if you agree

s1fr0 commented 1 year ago

From what I understood https://github.com/status-im/nwaku/pull/1373 is definitely an improvement, but was not the fix to this issue. The problem seems to be similar to https://github.com/status-im/nwaku/issues/1005, i.e. the relative path of the rlnlib. Is that right?

rymnc commented 1 year ago

Yes, and we have 2 options to solve the path issue -

LNSD commented 1 year ago

As you and @LNSD have mentioned, use rln as a static lib

This is a requirement from product for RLN productionalization

fryorcraken commented 1 year ago

Happy if this closed against an issue support of rln as static lib. Otherwise IMO it should remain open.

s1fr0 commented 1 year ago

Happy if this closed against an issue support of rln as static lib. Otherwise IMO it should remain open.

Done in https://github.com/status-im/nwaku/issues/1380

jm-clius commented 1 year ago

Closing, as #1380 has been opened