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

Clean up Makefile for rebuilding dependencies #486

Closed
wants to merge 1 commit into from

Conversation

saagarjha
Copy link
Contributor

This also allows the build to run in parallel.

@gnachman
Copy link
Owner

This has angered openssl, which, last I checked, is not compatible with parallel builds (in exactly what ways, I do not know! but this is very on brand for openssl):

% make deps
[a million lines]
cc  -I. -Iinclude -I.. -I../include -fPIC -arch x86_64 -I/opt/homebrew/Cellar/libevent/2.1.12/include -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAESNI_ASM -DVPAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPOLY1305_ASM -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib/engines-1.1\"" -D_REENTRANT -DNDEBUG  -MMD -MF crypto/ec/curve448/arch_32/f_impl.d.tmp -MT crypto/ec/curve448/arch_32/f_impl.o -c -o crypto/ec/curve448/arch_32/f_impl.o ../crypto/ec/curve448/arch_32/f_impl.c
../crypto/ec/curve448/arch_32/f_impl.c:13:10: fatal error: 'field.h' file not found
#include "field.h"
         ^~~~~~~~~
1 error generated.
make[1]: *** [crypto/ec/curve448/arch_32/f_impl.o] Error 1
make: *** [x86openssl] Error 2

@saagarjha
Copy link
Contributor Author

Yeah I noticed that I didn’t like that, that’s why I pulled out the configure step and forced it to be serial. Let me look into it more and see if I can reproduce

@saagarjha
Copy link
Contributor Author

Hmm I tried it a couple dozen times and could not reproduce. I wonder if you might have some cruft from a previous build attempt lying around? Can you try running make clean and then git clean -xfd in submodules/openssl and then trying again?

@gnachman
Copy link
Owner

Hmm I tried it a couple dozen times and could not reproduce. I wonder if you might have some cruft from a previous build attempt lying around? Can you try running make clean and then git clean -xfd in submodules/openssl and then trying again?

Still fails. Here's my log: https://gist.github.com/gnachman/bea5d119916e1bc240479cfc27155a03

Skipping the ARM build seems to fix the problem. I guess it's leaving some crap behind that breaks the x86 build.

@saagarjha
Copy link
Contributor Author

Hmm, I'm a bit confused by your log. When you run make deps is it running in parallel? Or is the normal build broken too? And your build seems to just be skipping over a lot of the dependencies (lots of "Nothing to be done"). Can you double-check that you ran make clean, and for good measure, git clean -xfd && git submodule foreach git clean -xfd? When I got that error it was because I had files left over but I want to make sure they're not just from previous builds with the old Makefiles.

Makefile Outdated
make libgit2

PATH=/usr/local/bin:${PATH} cd submodules/libgit2/build && ${CMAKE} -DBUILD_SHARED_LIBS=OFF -DCMAKE_OSX_ARCHITECTURES="x86_64;arm64" -DCMAKE_OSX_DEPLOYMENT_TARGET="10.14" -DCMAKE_INSTALL_PREFIX=../../../ThirdParty/libgit2 ..
PATH=/usr/local/bin:${PATH} cd submodules/libgit2/build && ${CMAKE} --build . --target install --parallel "$(sysctl -n hw.ncpu)"
Copy link
Owner

Choose a reason for hiding this comment

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

This needs double dollar signs $$(sysctl -n hw.ncpu) or else make evaluates it to empty string.

@gnachman
Copy link
Owner

This is failing because libgit2 doesn't find the right libssh library (the fat one). I spent some time trying to persuade it to use mine, going so far as to create a fake .pc file, but it appears to be an unwinnable war. I think libssh2 (and libssl and libcrypto at least) need to go through proper install procedures in a clean sandbox so it won't accidentally pick up homebrew stuff. I build the Python runtime this way, using a .sb file of:

(version 1)
(allow default)
(deny file* (subpath "/usr/local"))

I think you'll probably need to also deny /opt/homebrew and use make install on everything using a custom --prefix, and use lipo to fix up installed libraries in their proper locations. I'm still not 100% sure this would work because pkgconfig and automake are evil.

@saagarjha
Copy link
Contributor Author

To be entirely honest my personal copy builds with -DUSE_SSH=OFF (and -DUSE_ICONV=OFF) because I figured you had some special setup with your Homebrew to make it not do that, and I didn't want to mess with it. If you want to enable using the ones we build I think I could write a sandbox profile for it, but we'd also need to update the project to point at it since it currently doesn't have the search paths in pidinfo set up to consume the libraries. Should we go down that route?

@saagarjha
Copy link
Contributor Author

@gnachman pulled the latest changes. I think your Makefile had a couple of new mistakes (for example armlibssh2 builds for x86_64) and it didn't seem necessary anyways so I mostly just grabbed your -ld64 updates. While this warns in Xcode because they renamed it to -ld_classic I think we can deal with that later.

@gnachman
Copy link
Owner

@saagarjha Thanks for pointing that out. I've fixed the architecture issue and switch to the new linker flag. LMK if you find any other issues.

This also allows the build to run in parallel.
@saagarjha
Copy link
Contributor Author

Rebased your latest changes. Anything else I need to do for this PR?

@gnachman
Copy link
Owner

Thank you. Merged as 9b37706

@gnachman gnachman closed this Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants