Skip to content

Fix linter #120

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

Closed
wants to merge 2 commits into from
Closed

Fix linter #120

wants to merge 2 commits into from

Conversation

ystade
Copy link
Contributor

@ystade ystade commented Jun 3, 2025

Description

This PR fixes an issue that was only discovered in #982 in MQT Core. Before, pybind11 was installed in a different python environment than later used during the run of cpp-linter. Now, we use the same env.

Checklist:

  • The pull request only contains commits that are focused and relevant to this change.
  • I have added appropriate tests that cover the new/changed functionality.
  • I have updated the documentation to reflect these changes.
  • I have added entries to the changelog for any noteworthy additions, changes, fixes or removals.
  • I have added migration instructions to the upgrade guide (if needed).
  • The changes follow the project's style guidelines and introduce no new warnings.
  • The changes are fully tested and pass the CI checks.
  • I have reviewed my own code changes.

@ystade ystade self-assigned this Jun 3, 2025
@ystade ystade added continuous integration Anything related to the CI setup fix Fix for something that is not working labels Jun 3, 2025
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

What. This is something to look into further.
What you are doing here is exactly what the setup-uv option should accomplish.

Besides that, this PR contains some extra commits from the other branch apparently.

@ystade
Copy link
Contributor Author

ystade commented Jun 3, 2025

Oops, sorry. The other commits should not have been part of this PR here. They are removed now.

The issue is, that the cpp-linter sets up its own virtual environment venv and activates it. Hence, during linting it cannot find the pybind11 headers anymore. Setting up an virtual environment with the same name via uv and installing pybind11 there hopefully resolves the issue.

@ystade
Copy link
Contributor Author

ystade commented Jun 3, 2025

The issue is that one apparently can neither tell cpp-linter to use the existing venv nor to tell uv-setup how the virtual environment should be called.

@burgholzer
Copy link
Member

Oops, sorry. The other commits should not have been part of this PR here. They are removed now.

The issue is, that the cpp-linter sets up its own virtual environment venv and activates it. Hence, during linting it cannot find the pybind11 headers anymore. Setting up an virtual environment with the same name via uv and installing pybind11 there hopefully resolves the issue.

Ah. That makes more sense now.
Seems like a bit of an ugly workaround (depending on internals of the cpp-linter), but if it works, that's great.
Could we add some of the above in some form as an in-code comment so that we do not forget why this is there? Then, this is good to go.

- if: ${{ inputs.setup-python }}
name: Create virtual environment
run: |
${{ steps.setup-python.outputs.python-path }} -m venv venv

Check warning

Code scanning / CodeQL

Code injection Medium

Potential code injection in
${ steps.setup-python.outputs.python-path }
, which may be controlled by an external user.
@ystade
Copy link
Contributor Author

ystade commented Jun 4, 2025

Ok, this PR became superfluous. The workflow works just fine, there is no bug in the workflow. THe issue in the other PRs using this workflow were caused by a typo, i.e., -DBUILD_MQT_QMAP_BINDINGS=ON instead of -DBUILD_MQT_CORE_BINDINGS=ON (mind the QMAP and CORE).

@ystade ystade closed this Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous integration Anything related to the CI setup fix Fix for something that is not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants