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: static linking negentropy in ARM based mac #3046

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

gabrielmer
Copy link
Contributor

Description

Adding --passL:-L/opt/homebrew/lib/ when linking negentropy to fix error of missing crypto library in ARM based mac

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

It is that! Thanks for fixing it.

Copy link

github-actions bot commented Sep 20, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3046

Built from 41bf14c

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! Just minor comment

Makefile Outdated
@@ -185,7 +185,7 @@ $(LIBNEGENTROPY_FILE):
negentropy: | $(LIBNEGENTROPY_FILE)
## Pass libnegentropy and it's deps to linker.
$(eval LIBNEGENTROPY_PATH := $(shell if [ -f "$(LIBNEGENTROPY_FILE)" ]; then echo "$(LIBNEGENTROPY_FILE)"; else echo "./$(LIBNEGENTROPY_FILE)"; fi))
$(eval NIM_PARAMS += --passL:$(LIBNEGENTROPY_PATH) --passL:-lcrypto --passL:-lssl --passL:-lstdc++)
$(eval NIM_PARAMS += --passL:$(LIBNEGENTROPY_PATH) --passL:-lcrypto --passL:-lssl --passL:-lstdc++ --passL:-L/opt/homebrew/lib/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could be a bit more explicit and use detected_OS to give one value or another

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could be a bit more explicit and use detected_OS to give one value or another

I actually asked @NagyZoltanPeter about it and told me it doesn't hurt and no need to add an if condition for it. I have no specific preference, can do any of both ways :))

Copy link
Contributor

Choose a reason for hiding this comment

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

Well this is opted from the negentropy Makefile we had, homebrew is really just a linker hint, doing nothing harm on linux as there are no libs on such path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well this is opted from the negentropy Makefile we had, homebrew is really just a linker hint, doing nothing harm on linux as there are no libs on such path.

ah yes you are right that it doesn't cause any harm but I think is better to be explicit ( or nitpick hehe ;P )

@gabrielmer gabrielmer merged commit 256b785 into master Sep 20, 2024
10 of 11 checks passed
@gabrielmer gabrielmer deleted the fix-using-static-linking-in-mac branch September 20, 2024 12:41
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