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

build: fetch urcrypt instead of vendoring it #524

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

matthew-levan
Copy link
Contributor

As urcrypt now has its own repository, we can stop vendoring it and simply fetch it like any of our other third-party dependencies.

@matthew-levan matthew-levan requested a review from a team as a code owner September 27, 2023 01:27
@matthew-levan matthew-levan force-pushed the msl/fetch-urcrypt branch 3 times, most recently from 5793177 to a3d3c2f Compare September 27, 2023 15:17
joemfb
joemfb previously approved these changes Nov 9, 2023
Copy link
Member

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

this will do

@joemfb
Copy link
Member

joemfb commented Nov 9, 2023

@matthew-levan I seem to recall there being some question about the urcrypt header pattern here; has that been resolved? if so, feel free to merge.

@matthew-levan
Copy link
Contributor Author

@matthew-levan I seem to recall there being some question about the urcrypt header pattern here; has that been resolved? if so, feel free to merge.

Yea there were, but I never got around to resolving them. Looks like the build is working though in CI-- should we just merge and punt on the header particulars?

@joemfb
Copy link
Member

joemfb commented Nov 13, 2023

This fails on my machine:

Makefile.am: installing 'build-aux/depcomp'
Makefile.am:23: error: 'pkgconfig_DATA' is used but 'pkgconfigdir' is undefined
autoreconf: error: automake failed with exit status: 1

@matthew-levan
Copy link
Contributor Author

matthew-levan commented Nov 13, 2023 via email

@joemfb
Copy link
Member

joemfb commented Nov 13, 2023

I don't have a /usr/local/include directory. I checked out your branch and ran bazel build --clang_version=14.0.0 :urbit.

@matthew-levan
Copy link
Contributor Author

matthew-levan commented Nov 13, 2023 via email

@joemfb
Copy link
Member

joemfb commented Nov 13, 2023

MacOS 12.6.1. 2021 Macbook Pro (m1 max)

@matthew-levan
Copy link
Contributor Author

matthew-levan commented Nov 13, 2023 via email

@joemfb
Copy link
Member

joemfb commented Nov 15, 2023

After installing pkg-config via homebrew, i get a new error:

libtool: compile:  /usr/bin/clang -DHAVE_CONFIG_H -I. -I/private/var/tmp/_bazel_joemfb/f46182e3eb1bf4d7eff53357bd650651/sandbox/darwin-sandbox/3/execroot/__main__/external/aes_siv -I/private/var/tmp/_bazel_joemfb/f46182e3eb1bf4d7eff53357bd650651/sandbox/darwin-sandbox/3/execroot/__main__/bazel-out/darwin_arm64-opt/bin/external/aes_siv -I/private/var/tmp/_bazel_joemfb/f46182e3eb1bf4d7eff53357bd650651/sandbox/darwin-sandbox/3/execroot/__main__/bazel-out/darwin_arm64-opt/bin/external/openssl/openssl/include -I/private/var/tmp/_bazel_joemfb/f46182e3eb1bf4d7eff53357bd650651/sandbox/darwin-sandbox/3/execroot/__main__/bazel-out/darwin_arm64-opt/bin/external/urcrypt/urcrypt.ext_build_deps/openssl/include -I/private/var/tmp/_bazel_joemfb/f46182e3eb1bf4d7eff53357bd650651/sandbox/darwin-sandbox/3/execroot/__main__/bazel-out/darwin_arm64-opt/bin/external/urcrypt/urcrypt.ext_build_deps/secp256k1/include -march=native -std=c11 -Wextra -Wpedantic -Wall -Wall -g -O3 -MT keccak-tiny/libkeccak_tiny_la-keccak-tiny.lo -MD -MP -MF keccak-tiny/.deps/libkeccak_tiny_la-keccak-tiny.Tpo -c keccak-tiny/keccak-tiny.c -o keccak-tiny/libkeccak_tiny_la-keccak-tiny.o
clang: error: the clang compiler does not support '-march=native'
make[1]: *** [keccak-tiny/libkeccak_tiny_la-keccak-tiny.lo] Error 1

@matthew-levan
Copy link
Contributor Author

matthew-levan commented Nov 15, 2023 via email

@joemfb
Copy link
Member

joemfb commented Nov 15, 2023

This now works on my machine, but is broken on linux in CI.

Also, can you update INSTALL.md to mention the new pkg-config dependency for macos?

@matthew-levan matthew-levan force-pushed the msl/fetch-urcrypt branch 2 times, most recently from e7ba4f2 to cf391f7 Compare November 15, 2023 20:32
@matthew-levan matthew-levan requested a review from joemfb November 15, 2023 20:49
@matthew-levan
Copy link
Contributor Author

Ok I fixed the build, and it should work with compilers that don't support -march=native also.

@joemfb
Copy link
Member

joemfb commented Nov 15, 2023

After this update, the build failed with

error: possibly undefined macro: AC_MSG_NOTICE

After i installed autoconf-archive from homebrew, it builds correctly.

INSTALL.md Outdated Show resolved Hide resolved
Copy link
Member

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

this will do

@matthew-levan matthew-levan merged commit 185d6d3 into develop Nov 15, 2023
5 checks passed
@matthew-levan matthew-levan deleted the msl/fetch-urcrypt branch November 15, 2023 21:26
@joemfb joemfb restored the msl/fetch-urcrypt branch November 15, 2023 21:27
@joemfb joemfb deleted the msl/fetch-urcrypt branch November 15, 2023 21:28
joemfb added a commit that referenced this pull request Jan 25, 2024
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.

2 participants