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

Update install_deps.sh to use principia_make.sh from dependencies #2523

Merged
merged 8 commits into from
Apr 12, 2020

Conversation

ts826848
Copy link
Contributor

@ts826848 ts826848 commented Apr 10, 2020

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:

Undefined symbols for architecture x86_64:
  "google::FlagRegisterer::FlagRegisterer<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >(char const*, char const*, char const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*)", referenced from:
      __GLOBAL__sub_I_logging.cc in libglog.a(libglog_la-logging.o)
      __GLOBAL__sub_I_vlog_is_on.cc in libglog.a(libglog_la-vlog_is_on.o)
  "google::FlagRegisterer::FlagRegisterer<bool>(char const*, char const*, char const*, bool*, bool*)", referenced from:
      __GLOBAL__sub_I_logging.cc in libglog.a(libglog_la-logging.o)
      __GLOBAL__sub_I_utilities.cc in libglog.a(libglog_la-utilities.o)
  "google::FlagRegisterer::FlagRegisterer<int>(char const*, char const*, char const*, int*, int*)", referenced from:
      __GLOBAL__sub_I_logging.cc in libglog.a(libglog_la-logging.o)
      __GLOBAL__sub_I_vlog_is_on.cc in libglog.a(libglog_la-vlog_is_on.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [bin/tools] Error 1

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.

Alex Wang added 2 commits April 10, 2020 00:56
zfp.h is located at include/zfp.h, not include/zfp/zfp.h.
Alex Wang added 3 commits April 10, 2020 03:04
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
Copy link
Member

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?

@ts826848 ts826848 marked this pull request as draft April 10, 2020 07:38
@ts826848 ts826848 marked this pull request as ready for review April 10, 2020 07:40
@ts826848 ts826848 changed the title [WIP] zfp-related fixes on macOS zfp-related fixes on macOS Apr 10, 2020
@ts826848 ts826848 changed the title zfp-related fixes on macOS Build fixes on macOS Apr 10, 2020
Match flags used in build pipeline
Copy link
Member

@eggrobin eggrobin left a 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:

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.

@mockingbirdnest mockingbirdnest deleted a comment from pleroy Apr 10, 2020
Rewrite to work with principia_make.sh in dependency repositories.
General structure given by egg at [0].

[0]: mockingbirdnest#2523 (review)
@ts826848 ts826848 changed the title Build fixes on macOS Update install_deps.sh to use principia_make.sh from dependencies Apr 12, 2020
@ts826848
Copy link
Contributor Author

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.

Copy link
Member

@eggrobin eggrobin left a 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.

@eggrobin eggrobin added the LGTM label Apr 12, 2020
@eggrobin eggrobin merged commit 380dfa1 into mockingbirdnest:master Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants