-
Notifications
You must be signed in to change notification settings - Fork 69
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
Update install_deps.sh to use principia_make.sh from dependencies #2523
Conversation
zfp.h is located at include/zfp.h, not include/zfp/zfp.h.
@@ -107,3 +107,17 @@ git pull | |||
cmake -DCMAKE_C_COMPILER:FILEPATH=`which clang` -DCMAKE_CXX_COMPILER:FILEPATH=`which clang++` -DCMAKE_C_FLAGS="${C_FLAGS}" -DCMAKE_CXX_FLAGS="${CXX_FLAGS}" -DCMAKE_LD_FLAGS="${LD_FLAGS}" -DBENCHMARK_ENABLE_GTEST_TESTS=OFF | |||
make -j8 | |||
popd | |||
|
|||
if [ ! -d "zfp" ]; then | |||
git clone "https://github.com/LLNL/zfp.git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the other dependencies, we don’t live at head, we have our own fork: https://github.com/mockingbirdnest/zfp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by ba56718
Try to avoid unnecessary recompiles on subsequent runs of install_deps.sh
This reverts commit 3c8dad0. The #include was correct. zfp/zfp.h was added in Principia's fork of zfp, so fixing the clone solves the problem.
cmake -DCMAKE_C_COMPILER:FILEPATH=`which clang` -DCMAKE_CXX_COMPILER:FILEPATH=`which clang++` -DCMAKE_C_FLAGS="${C_FLAGS}" -DCMAKE_CXX_FLAGS="${CXX_FLAGS}" -DCMAKE_LD_FLAGS="${LD_FLAGS}" -DBUILD_SHARED_LIBS=OFF .. | ||
make -j8 | ||
popd | ||
popd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, looking at the pipelines, they do
mkdir build; cd build; cmake -DZFP_WITH_OPENMP=OFF -DBUILD_SHARED_LIBS=OFF ..; make
on both Linux and Mac, which seems bad (in particular it does not pass DCMAKE_C_COMPILER
etc.).
We should really unify install_deps.sh
with the code run by the pipelines in some way; maybe via https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/multi-repo-checkout?view=azure-devops?
@pleroy, any thoughts?
Match flags used in build pipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some context here: since the beginning of the year, we have been using azure pipelines to streamline releasing binaries on Linux and macOS. As a result, install_deps
has mostly fallen in disrepair, while a lot of what it did moved into inscrutable azure pipeline configuration.
In order to make that more reviewable (it turns out that we were building ZFP with GCC!) and to make it usable when building locally, I moved the build command for each dependency, as well as any override to the common variables, into files in the repository, principia_make.sh
and principia_variable_overrides
:
- principia_make glog#5
- principia_make benchmark#3
- principia_make protobuf#4
- principia_make abseil-cpp#1
- principia_make gipfeli#1
- principia_make zfp#3
- principia_make #2525
As a result, this pull request now needs to change from a minor fix to a complete rewrite of install_deps
.
For each of the dependencies listed above, the following needs to be done:
Set the variables
AGENT_OS=$(uname -s)
PRINCIPIA_C_FLAGS=-fPIC -O3 -g -DNDEBUG
PRINCIPIA_CXX_FLAGS=-std=c++1z
PRINCIPIA_LD_FLAGS=-stdlib=libc++
PRINCIPIA_MACOS_CXX_FLAGS=-D_LIBCPP_STD_VER=16
PRINCIPIA_MACOS_VERSION_MIN=10.12
If present, run ./principia_variable_overrides.sh
(from the dependency directory).
Run:
if [ "$AGENT_OS" == "Darwin" ]; then
C_FLAGS="$PRINCIPIA_C_FLAGS -mmacosx-version-min=$PRINCIPIA_MACOS_VERSION_MIN -arch x86_64"
CXX_FLAGS="$PRINCIPIA_CXX_FLAGS $PRINCIPIA_MACOS_CXX_FLAGS"
elif [ "$AGENT_OS" == "Linux" ]; then
C_FLAGS="$PRINCIPIA_C_FLAGS -m64"
CXX_FLAGS="$PRINCIPIA_CXX_FLAGS"
else
C_FLAGS="$PRINCIPIA_C_FLAGS"
CXX_FLAGS="$PRINCIPIA_CXX_FLAGS"
fi
LD_FLAGS="$C_FLAGS $PRINCIPIA_LD_FLAGS"
CXX_FLAGS="$CXX_FLAGS $LD_FLAGS"
(probably with ${MEOW?} instead of $MEOW everywhere for safety).
Run ./principia_make
.
Rewrite to work with principia_make.sh in dependency repositories. General structure given by egg at [0]. [0]: mockingbirdnest#2523 (review)
The changes have been made. I've successfully built Principia on my mac, and all the tests that passed before still pass. Hopefully things will work fine on Linux too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, added some comments to the script.
zfp does not support in-tree CMake builds, so the additional
build
directory is needed in install_deps.sh.I'm not entirely sure changing#include "zfp/zfp.h"
to#include "zfp.h"
won't break anything on Windows. I have to reinstall VS 2017 on my machine sometime tomorrow.I still get build errors with these two commits, but they seem to be unrelated:
This is fixable by adding
-lgflags
to the Makefile since I already had gflags installed through Homebrew, but I'm not sure how you guys want this to be addressed. Should I specify gflags as a build dependency in install_deps.sh along with clang/libc++-dev and friends, or do you want it built as a third-party dep?I still need to try this on Windows too to see if it's an issue on there.