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

Fixes CMake version > 3.12 python-binding build failure #13631

Merged
merged 1 commit into from May 11, 2022

Conversation

CorbinFoucart
Copy link
Contributor

My build (deal.II 9.3.3, Ubuntu 18.04) with python-bindings was failing at the cmake . stage. Looking at the CMakeLists.txt file and comparing against the CMake documentation, it seems that this is due to a deprecation of FindPythonInterp for newer versions of CMake.

In order to fix the issue, I added a check based on the User's CMake version, since deal.II supports older versions. Using the new CMake functionality sets different CMake variables for Python version, so I set the existing variables based on the new variables as well. They're needed later in the file.

After making these changes, I'm able to build the library, the quicktests pass, and I'm able to import the PyDealII python library from an interpreter.

Copy link
Member

@Rombur Rombur 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. Thanks for the PR.

@Rombur
Copy link
Member

Rombur commented Apr 22, 2022

/rebuild

@peterrum
Copy link
Member

@CorbinFoucart CI is not happy about your git name. Could you check the following page: https://github.com/dealii/dealii/wiki/Commit-authorship.

@peterrum peterrum added this to the Release 9.4 milestone Apr 24, 2022
@Rombur
Copy link
Member

Rombur commented Apr 29, 2022

@CorbinFoucart can you take a look @peterrum's comment. Let us know if you need any help.

@bangerth
Copy link
Member

bangerth commented May 6, 2022

@CorbinFoucart Ping?

@CorbinFoucart
Copy link
Contributor Author

Hi everyone, thanks for the ping. I will fix this in a day or so.

@CorbinFoucart
Copy link
Contributor Author

CorbinFoucart commented May 7, 2022

I've amended the commit to include my full name. Is this fine, or should I open a new pull request?

When I run git log, I see

commit 0309955a9e381d66a82ac309bc8b151588380ea2 (HEAD -> python3_binding_cmake_build_fix, origin/python3_binding_cmake_build_fix)
Merge: 6ec688ec82 6d2b712476
Author: Corbin Foucart <corbin.foucart@gmail.com>
Date:   Sat May 7 15:55:36 2022 -0400

    Merge branch 'python3_binding_cmake_build_fix' of github.com:CorbinFoucart/dealii into python3_binding_cmake_build_fix

commit 6ec688ec82cc591a4869beb335c439bd88b3ef65
Author: Corbin Foucart <corbin.foucart@gmail.com>
Date:   Fri Apr 22 01:56:30 2022 -0400

    Fixes CMake version > 3.12 python-binding build failure due to deprecated FindPythonInterp call.

commit 6d2b712476e698aa61c5feef9898b5355a607525
Author: Corbin <corbin.foucart@gmail.com>
Date:   Fri Apr 22 01:56:30 2022 -0400

    Fixes CMake version > 3.12 python-binding build failure due to deprecated FindPythonInterp call.

commit 3efeff2bc1bde872b103867c2fa90ae5b7d80a5d (origin/master, origin/HEAD, master)
Merge: 4bbf721af4 3fcb617604
Author: David Wells <drwells@email.unc.edu>
Date:   Thu Apr 21 09:52:03 2022 -0400

    Merge pull request #13629 from kronbichler/fix_typos
    
    Fix typo in variable name

It looks like amending the commit created another commit, after which the author name is correct. However, it looks like the CI check still fails below.

@Rombur
Copy link
Member

Rombur commented May 9, 2022

It looks like amending the commit created another commit, after which the author name is correct.

That's the problem. You will need to squash the two commits into one to fix that. Do you know how to do it? You can take a look at https://stackoverflow.com/a/5189600 After that you will need to use git push -f to force the push because you changed the history.

@CorbinFoucart CorbinFoucart force-pushed the python3_binding_cmake_build_fix branch from 0309955 to 3c0a6bb Compare May 10, 2022 17:49
@Rombur
Copy link
Member

Rombur commented May 11, 2022

Thanks for the patch @CorbinFoucart !

@Rombur Rombur merged commit 29c7743 into dealii:master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants