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.

Sign up for GitHub

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

Jump to bottom

Fixes for compilation on Mac / Darwin #2450

Merged
merged 11 commits into from
Oct 15, 2021
Merged

Fixes for compilation on Mac / Darwin #2450

merged 11 commits into from
Oct 15, 2021

Conversation

bendudson
Copy link
Contributor

@bendudson bendudson commented Oct 5, 2021 •
edited

Compiling with Apple Clang 12.0.0 on MacOS.

Compile flags to disable components which currently don't compile and/or link on MacOS have been put in the manual:

cmake . -B build -DBOUT_ENABLE_BACKTRACE=Off -DBUILD_SHARED_LIBS=Off -DBOUT_USE_NLS=Off -DBOUT_USE_UUID_SYSTEM_GENERATOR=Off
cd build
make

Clang has found another possible bug:

BOUT-dev/src/mesh/interpolation_xz.cxx:39:48: warning: returning address of local temporary object
      [-Wreturn-stack-address]
const char* strLocation(CELL_LOC loc) { return toString(loc).c_str(); }

@bendudson
Replace ::finite with std::isfinite
87fb21c 
Included in C++11, marked deprecated in Clang compiler on Darwin
(Apple clang version 12.0.0)
@bendudson
Remove unused private variable
270744c 
ncxx4 not using `appending` variable
@bendudson
Use std::abs rather than std::fabs for complex numbers
fb45949 
https://en.cppreference.com/w/cpp/numeric/complex/abs

It appears that std::fabs may have an implementation for complex,
but this is implementation specific. std::abs is in the standard.
@bendudson
Include missing <array> header
639c152 
Incomplete type causes trouble for constexpr values when compiled
with Apple Clang (12.0.0).
github-actions[bot]
github-actions bot reviewed Oct 5, 2021
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/fileio/impls/netcdf4/ncxx4.cxx Outdated Show resolved Hide resolved
src/fileio/impls/netcdf4/ncxx4.cxx Outdated
@@ -802,7 +802,7 @@ bool Ncxx4::write(BoutReal *data, const char *name, int lx, int ly, int lz) {
}

for(int i=0;i<lx*ly*lz;i++) {
if(!finite(data[i]))
if(!std::isfinite(data[i]))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

if(!std::isfinite(data[i]))
                           ^
                            {

src/fileio/impls/netcdf4/ncxx4.cxx Outdated Show resolved Hide resolved
src/fileio/impls/netcdf4/ncxx4.cxx Outdated
@@ -853,7 +853,7 @@ bool Ncxx4::write_perp(BoutReal *data, const std::string& name, int lx, int lz)
}

for(int i=0;i<lx*lz;i++) {
if(!finite(data[i]))
if(!std::isfinite(data[i]))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

if(!std::isfinite(data[i]))
                           ^
                            {

src/fileio/impls/netcdf4/ncxx4.cxx Outdated Show resolved Hide resolved
src/fileio/impls/netcdf4/ncxx4.cxx Outdated
@@ -1120,7 +1120,7 @@ bool Ncxx4::write_rec(BoutReal *data, const char *name, int lx, int ly, int lz)
}

for(int i=0;i<lx*ly*lz;i++) {
if(!finite(data[i]))
if(!std::isfinite(data[i]))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

if(!std::isfinite(data[i]))
                           ^
                            {

src/fileio/impls/netcdf4/ncxx4.cxx Outdated Show resolved Hide resolved
src/fileio/impls/netcdf4/ncxx4.cxx Outdated
@@ -1188,7 +1188,7 @@ bool Ncxx4::write_rec_perp(BoutReal *data, const std::string& name, int lx, int
}

for(int i=0;i<lx*lz;i++) {
if(!finite(data[i]))
if(!std::isfinite(data[i]))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

if(!std::isfinite(data[i]))
                           ^
                            {

@ZedThree
Copy link
Member

ZedThree commented Oct 6, 2021

I think probably strLocation is deprecated in next, replaced by toString? #2291 should remove it, if so.

ZedThree
ZedThree previously approved these changes Oct 7, 2021
View reviewed changes
@bendudson
Avoid map operator= for Options::attributes
fba50c3 
For reasons I don't understand, this fails on Apple Clang 12.
Tried both -std=c++14 and -std=c++17.

Replacing with an insertion works.
@bendudson
Add some documentation on MacOS builds
781c192 
Debugging hint (VERBOSE=1), and cmake settings for compilation.
@bendudson
Add some documentation on MacOS builds
8cdf977 
Debugging hint (VERBOSE=1), and cmake settings for compilation.
dschwoerer
dschwoerer reviewed Oct 8, 2021
View reviewed changes
manual/sphinx/user_docs/installing.rst
which are being issued by adding `VERBOSE=1` to the make command i.e. in the build
directory running::

$ make VERBOSE=1
Copy link
Contributor

Choose a reason for hiding this comment

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

cmake --build . -v does work as well

dschwoerer
dschwoerer previously approved these changes Oct 8, 2021
View reviewed changes
@bendudson
Copy link
Contributor Author

Some progress: The link command ends with

libfftw3.dylib -framework Accelerate -lm -ldl /Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk/System/Library/Frameworks/CoreFoundation.framework

That last /Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk/System/Library/Frameworks/CoreFoundation.framework is a directory not a library.
Any idea how to debug CMake to see where this flag might be coming from? (@ZedThree ?)

@bendudson
Clang tidy suggestions
2cb5a29 
Braces on conditionals, some spacing
@bendudson
Add verbose CMake build docs
2104c3b 
Thanks to @dschworer, cmake -v flag for additional debugging information
@ZedThree
Copy link
Member

Pretty sure it's coming from FindLibuuid.cmake:

BOUT-dev/cmake/FindLibuuid.cmake

Lines 31 to 43 in 26d8341

if (APPLE)
find_library(CFLIB CoreFoundation)
find_package_handle_standard_args(Libuuid DEFAULT_MSG CFLIB)
mark_as_advanced(${CFLIB})
if (Libuuid_FOUND AND NOT TARGET Libuuid::libuuid)
add_library(Libuuid::libuuid UNKNOWN IMPORTED)
set_target_properties(Libuuid::libuuid PROPERTIES
IMPORTED_LOCATION ${CFLIB}
)
endif()
return()
endif ()

It looks like importing Mac frameworks is a complicated business:

Easiest fix is to just delete that whole bit -- not having a system UUID generator is not a massive loss, we have a fallback solution anyway.

Some general points on debugging CMake files:

@bendudson
Disable UUID for Apple
e1e4b37 
Modifying instructions, tested on Darwin

   cmake . -B build -DBOUT_ENABLE_BACKTRACE=Off -DBUILD_SHARED_LIBS=Off -DBOUT_USE_NLS=Off -DBOUT_USE_UUID_SYSTEM_GENERATOR=Off
   cd build
   make

[skip ci]
@bendudson
Copy link
Contributor Author

Thanks @ZedThree that worked! I've updated the documentation rather than modify the CMakelists.txt file

@bendudson bendudson merged commit 27e1293 into next Oct 15, 2021
@bendudson bendudson deleted the next-darwin-fixes branch October 15, 2021 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@ZedThree ZedThree ZedThree left review comments

@dschwoerer dschwoerer dschwoerer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bendudson @ZedThree @dschwoerer