-
Notifications
You must be signed in to change notification settings - Fork 54
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
Added ubuntu 16 support to solve #16 #17
Conversation
…PETSC_DIR/$PETSC_ARCH as ubuntu needs the latter.
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.
Apparently I never "submitted" my review so it has been "pending" all this time. (Oops.)
This commit does more than one thing and creates some maintenance concerns going forward. Can you make it more clear exactly what problem is being solved? And if you need a libpetsc_real
then please make that a different commit -- it would need to find a different petscconf.h
so it isn't trivial or orthogonal.
@@ -47,9 +47,18 @@ else() | |||
endforeach() | |||
endif() | |||
|
|||
macro (check_if_arch dir) | |||
set (PETSC_TMP "${PETSC_DIR}/") |
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.
CMake functions normally take their return variable as an argument, rather than setting a global.
@@ -47,9 +47,18 @@ else() | |||
endforeach() | |||
endif() | |||
|
|||
macro (check_if_arch dir) |
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.
CMake macros are global, so we need to namespace.
@@ -224,6 +235,10 @@ show : | |||
else () | |||
set (PETSC_LIBRARY_VEC "NOTFOUND" CACHE INTERNAL "Cleared" FORCE) # There is no libpetscvec | |||
petsc_find_library (SINGLE petsc) | |||
# Hack : for ubuntu with real in the architecture | |||
if (PETSC_LIBRARY_SINGLE STREQUAL "PETSC_LIBRARY_SINGLE-NOTFOUND") |
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.
Better to use a boolean test instead of STREQUAL
. But if they are distributing multiple versions like this, we need to also find a compatible petscconf.h
. I don't see that, so your change here will produce broken code.
macro (check_if_arch dir) | ||
set (PETSC_TMP "${PETSC_DIR}/") | ||
if (EXISTS "${PETSC_DIR}/${PETSC_ARCH}/${dir}") | ||
set (PETSC_TMP "${PETSC_TMP}/${PETSC_ARCH}") |
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.
Why do you need this? If PETSC_ARCH
is unset and there is no subdirectory then the old version just works. If PETSC_ARCH
is explicitly set to something that does not exist (or is corrupt) then we should get an error instead of succeeding with no PETSC_ARCH
.
Sorry for the late reply. With the pull requestion #18 , my problem is solved so there is no need for this patch anymore. I just need to set PETSC_DIR=/usr/lib/petscdir/3.6.2/x86_64-linux-gnu-real ant it works. |
Here is my try to solve the issue #16 with the new directories : we add PETSC_DIR/$PETSC_ARCH to the search.