Skip to content
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

fix(Makefile): error out if rln-keystore-generator not compiled with rln flag #1960

Merged
merged 2 commits into from
Aug 29, 2023

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Aug 28, 2023

Description

This pr is associated to #1956 (comment)
We need to ensure that RLN is set to true when compiling the rln-keystore-generator target

Changes

  • Prevents rebuilding of rln by fixing the LIBRLN_BUILDDIR
  • conditional compilation of rln-keystore-generator

@rymnc rymnc self-assigned this Aug 28, 2023
@rymnc rymnc requested review from alrevuelta and vpavlin August 28, 2023 12:34
Makefile Outdated
Comment on lines 188 to 193
ifeq ($(RLN),true)
echo -e $(BUILD_MSG) "build/$@" && \
$(ENV_SCRIPT) nim rln_keystore_generator $(NIM_PARAMS) $(EXPERIMENTAL_PARAMS) waku.nims
else
$(error RLN is not true. To build rln-keystore-generator, set RLN=true.)
endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if there's a better way to make RLN default to true while building this target - thoughts @vpavlin?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! good idea, i'll add that
thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3a7ebc4

@github-actions
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1960

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of erroring out, cant we just set RLN=true if "make rln-keystore-generator"?

@rymnc
Copy link
Contributor Author

rymnc commented Aug 28, 2023

instead of erroring out, cant we just set RLN=true if "make rln-keystore-generator"?

yeah, not sure how to

@alrevuelta
Copy link
Contributor

seems that by default its compiled with TRACE, so perhaps we should add this to the instructions.

make rln-keystore-generator RLN=true LOG_LEVEL=info

@rymnc
Copy link
Contributor Author

rymnc commented Aug 29, 2023

seems that by default its compiled with TRACE, so perhaps we should add this to the instructions.

make rln-keystore-generator RLN=true LOG_LEVEL=info

Yeah im working on the readme for it

@alrevuelta
Copy link
Contributor

btw, it works fine but I see execution error: metadata does not exist in every run.

@rymnc
Copy link
Contributor Author

rymnc commented Aug 29, 2023

btw, it works fine but I see execution error: metadata does not exist in every run.

because when the tree has just been created, the metadata does not exist when we try to fetch it.

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@rymnc rymnc merged commit ac25855 into master Aug 29, 2023
@rymnc rymnc deleted the fix-rln-recomp-makefile branch August 29, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants