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

disable CXX/CC for deal.II itself #159

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

tjhei
Copy link
Member

@tjhei tjhei commented Jan 6, 2021

As described in the comment, it is not ideal to set CXX/CC.

@koecher
Copy link
Contributor

koecher commented Jan 8, 2021

Strange problem. Obviously this should resolve the issue reported in the comments.

I'm wondering if it will work for my compiler setting:
I have system installed gcc, which I don't use (old version, or version may switch with system updates).
With an environment module I set a user-specific gcc (non-standard location) and user-specific mpi with that. If now the C/CXX changes to the system gcc, my mpicxx (still given to deal.II) is compiled against a "wrong" base compiler. Maybe the build will work (since both base compilers are gcc in different version) but something strange can still happen.

I suggest to introduce (additionally) a new variable for the default parameter input / configuration file, something like "reset C / CXX compiler for deal.II" (default value: false)
and, as second security of the unset C, CXX, ...,
save the original C, CXX, FC compilers before a package run and reset them afterwards (as we do for CONFOPTS in candi.sh).

@tjhei
Copy link
Member Author

tjhei commented Jan 10, 2021

With an environment module I set a user-specific gcc (non-standard location) and user-specific mpi with that.

I assume you are running CXX=/some/other/mpicxx ./candi.sh and that mpi wrapper uses your user-installed gcc? You are right, this doesn't work correctly.

It seems to me that one needs to set both of them correctly:

CXX=g++-9 cmake -D MPI_CXX_COMPILER=/my/custom/mpicxx ...

Would you be opposed to introducing a way to specify the c++ compiler and the c++ MPI wrapper (maybe in candi.cfg or in the env?)? For example:
If only CXX is set, assume CXX is set to an MPI wrapper (and behave as before). If CXX and MPI_CXX is set, assume that CXX is the compiler and MPI_CXX is the MPI wrapper.

@tjhei
Copy link
Member Author

tjhei commented Jan 15, 2021

Let's wait for dealii/dealii#11478

@koecher
Copy link
Contributor

koecher commented Jan 20, 2021

With an environment module I set a user-specific gcc (non-standard location) and user-specific mpi with that.

I assume you are running CXX=/some/other/mpicxx ./candi.sh and that mpi wrapper uses your user-installed gcc? You are right, this doesn't work correctly.

Indeed, but I set the environment variables for the compiler and mpi-compiler with environment modules. I don’t think that can work, but with my approach given above, it shouldn’t be a problem.

It seems to me that one needs to set both of them correctly:

CXX=g++-9 cmake -D MPI_CXX_COMPILER=/my/custom/mpicxx ...

Would you be opposed to introducing a way to specify the c++ compiler and the c++ MPI wrapper (maybe in candi.cfg or in the env?)? For example:
If only CXX is set, assume CXX is set to an MPI wrapper (and behave as before). If CXX and MPI_CXX is set, assume that CXX is the compiler and MPI_CXX is the MPI wrapper.

Not really, this is so non-standard that I don’t think this will work for the future. Why not going for my approach, if something goes wrong, one can tryout to reset CXX/CC in the config file, if this also doesn’t work, one needs to find another solution.

We should wait for a potential patch for deal.II of course.

@tjhei
Copy link
Member Author

tjhei commented May 31, 2021

We should wait for a potential patch for deal.II of course.

It seems like we got to a conclusion in deal.II but the subtle issue remains: If you configure deal.II using candi on Ubuntu 20.04, MPI_INCLUDE_DIRS is not set correctly. You can compile&run just fine, except that my IDE (qtcreator) is missing the include path for code completion.
I am not sure if the same holds on other OSes/setups, but I would not be surprised. This is a pretty common situation, so hiding the correct solution behind a flag is not ideal.
I will need to think about this some more...

@koecher
Copy link
Contributor

koecher commented Jun 1, 2021

well, for my user compiled mpi-compilers I set the environment variables:

  • MPI_INCLUDE
  • MPI_HOME
  • MPI_LIB
  • MPI_BIN

and some others.

What do you mean by MPI_INCLUDE_DIRS is not set correctly? An empty string, or, even worst, a wrong string? If it is an empty string, one can set up a warning, that you should set this environment variable correctly.

@koecher
Copy link
Contributor

koecher commented Jun 17, 2021

@tjhei @gfcas I'm fine with that PR, if it helps you.

@ghost
Copy link

ghost commented Jun 17, 2021

@tjhei @koecher Yes, this patch makes candi working for 9.3.0 (just tested again). However, I noticed that it possibly may be not necessary to set the MPI_CC_COMPILER as I get the following message after dealii configuration:

CMake Warning:
Manually-specified variables were not used by the project:

MPI_CC_COMPILER


################################################################################
#Choose general configuration and components of deal.II

CONFOPTS=" \
-D CMAKE_BUILD_TYPE=DebugRelease \
-D DEAL_II_WITH_MPI:BOOL=ON \
-D MPI_CXX_COMPILER=${CXX} \
-D MPI_CC_COMPILER=${CC} \
Copy link
Contributor

Choose a reason for hiding this comment

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

@gfcas This line should be removed, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

precisely line 22 here

Copy link
Member Author

Choose a reason for hiding this comment

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

that was a typo, fixed.

As described in the comment, it is not ideal to set CXX/CC.
@tjhei
Copy link
Member Author

tjhei commented Jun 18, 2021

I will need to do some additional testing...

@ghost ghost mentioned this pull request Jun 18, 2021
23 tasks
@tjhei
Copy link
Member Author

tjhei commented Jun 19, 2021

To conclude:

I think this is good to go.

@koecher koecher merged commit c6943d0 into dealii:master Jun 21, 2021
tjhei added a commit to tjhei/candi that referenced this pull request Jun 28, 2021
Setting the compiler wrapper in CMake instead of in CC and CXX is
recommended, but only works for recent CMake versions (one configuration
I tested requires CMake 3.20 for the intel compiler).
With 9.3.1 deal.II supports setting CXX=mpicxx again, so only do the
above when a setting in candi.cfg is specified.
Works around dealii#202
and fixes feature introduced in dealii#159
tjhei added a commit that referenced this pull request Jun 28, 2021
Setting the compiler wrapper in CMake instead of in CC and CXX is
recommended, but only works for recent CMake versions (one configuration
I tested requires CMake 3.20 for the intel compiler).
With 9.3.1 deal.II supports setting CXX=mpicxx again, so only do the
above when a setting in candi.cfg is specified.
Works around #202
and fixes feature introduced in #159
@koecher koecher mentioned this pull request Jul 9, 2021
16 tasks
maxrudolph pushed a commit to maxrudolph/candi that referenced this pull request Nov 2, 2022
Setting the compiler wrapper in CMake instead of in CC and CXX is
recommended, but only works for recent CMake versions (one configuration
I tested requires CMake 3.20 for the intel compiler).
With 9.3.1 deal.II supports setting CXX=mpicxx again, so only do the
above when a setting in candi.cfg is specified.
Works around dealii#202
and fixes feature introduced in dealii#159
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants