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

Repairing warnings and error regarding Version.h by using pkgconfig to link to preCICE #149

Merged
merged 27 commits into from
Aug 25, 2022

Conversation

IshaanDesai
Copy link
Member

@IshaanDesai IshaanDesai commented Aug 22, 2022

This PR resolves a warning and an error in the action set of build-and-test. The warning was:

warning: cyprecice/cyprecice.pyx:1211:57: local variable '_value' referenced before assignment

and the error was

/home/runner/work/python-bindings/python-bindings/precice/SolverInterface.hpp:7:10: fatal error: precice/Version.h: No such file or directory
    7 | #include "precice/Version.h"
      |          ^~~~~~~~~~~~~~~~~~~

@IshaanDesai IshaanDesai self-assigned this Aug 22, 2022
@IshaanDesai IshaanDesai changed the title Assign value to _value in read_scalar_data function to avoid compiler warning Repairing warnings related to variables and error regarding Version.h in several actions Aug 22, 2022
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
@IshaanDesai IshaanDesai changed the title Repairing warnings related to variables and error regarding Version.h in several actions Repairing warnings related to variables and error regarding Version.h by using pkgconfig to link to preCICE Aug 24, 2022
@IshaanDesai IshaanDesai changed the title Repairing warnings related to variables and error regarding Version.h by using pkgconfig to link to preCICE Repairing warnings and error regarding Version.h by using pkgconfig to link to preCICE Aug 24, 2022
@IshaanDesai
Copy link
Member Author

In finding a solution to include the generated header Version.h in the mock testing workflow, the Python package pkgconfig is used to link the bindings to preCICE. This implementation was already discussed and partially done in #135, and a full solution in consultation with @fsimonis is done here.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Overall I think using pkgconfig makes a lot of sense here.

The issue with precice/precice vs. precice/precice:2.4.0 might be an upstream issue, but maybe this is also an argument to change things upstream?

One thing that might be affected by this change: Do you need updates for the spack recipe?

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/run-solverdummy.yml Show resolved Hide resolved
@fsimonis
Copy link
Member

One thing that might be affected by this change: Do you need updates for the spack recipe?

Yes.
Currently, spack is failing due to py-pkgconfig missing during build. I assume this based on the build failing during processing of the pyproject.toml file.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

I still have some minor points w.r.t to the mocking of preCICE. Can you try to remove the build process of preCICE and the installation of the dependencies, if it is not absolutely needed? I think it should work without building preCICE.

.github/workflows/build-and-test.yml Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
.github/workflows/build-and-test.yml Show resolved Hide resolved
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

After some clarifications I cannot find anything to complain anymore. Thanks for the explanations @IshaanDesai 🙏

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