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

Add C++ compiler as dependency #140

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Add C++ compiler as dependency #140

merged 1 commit into from
Apr 12, 2022

Conversation

ajaust
Copy link
Collaborator

@ajaust ajaust commented Mar 29, 2022

This mention a C++ compiler as dependency for building the bindings in the README. It was pointed out that installation of the bindings fails, if there is no C++ compiler installed.

Do we actually need MPI as a dependency as well?

Mention a C++ compiler as dependency for building the bindings.
Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

Makes sense to have a note that a working C++ compiler needs to exist on the system. Regarding having MPI as a dependency, things are a bit unclear. In the bindings we import mpi4py but never really use it, so I am also unsure whether we really need MPI or even mpi4py as a dependency. @BenjaminRodenberg worked on #101 some time ago, which removes the MPI dependency altogether.

@BenjaminRodenberg
Copy link
Member

If I understand this right, this PR is directly related to precice/precice#1225. Because it already caused problems that the C++ compiler is undocumented, I think it's a good idea to add a note in the documentation.

Finding a proper treatment for C++ dependencies of this package is a bigger issue. Checking for cython is something we are also missing currently, as far as I see it. But I am not sure whether we actually want to do this (see, for example https://github.com/pandas-dev/pandas/pull/9881/files#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7).

Let's document the C++ compiler, but not change the actual implementation because this might cause more problems than it would actually solve.

@ajaust
Copy link
Collaborator Author

ajaust commented Apr 12, 2022

Yes, it is linked to the issue in the preCICE repository. I agree that that it should be enough to document the need for a C++ compiler and changing the whole installation process would be too much at the moment.

I think the main problem was that building the bindings fails in a somewhat non-intuitive way. pip installs the dependencies, including Cython, but then the build process for the bindings fails. Therefore, having the dependency on the compiler documented should be helpful even though this might be a rather rare problem.

@IshaanDesai IshaanDesai merged commit 8eee607 into develop Apr 12, 2022
@IshaanDesai IshaanDesai deleted the ajaust-patch-1 branch August 9, 2023 17:56
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.

3 participants