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

Implement SYSTEM handler for cmake dependencies. #13576

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luigifcruz
Copy link

The current dependency handler for CMake configuration files doesn't handle including paths that were marked as SYSTEM. This can lead to header conflicts on certain occasions and ultimately hard-to-debug compilation failures.

In my particular case, my program compilation fails when importing anything built with the NVIDIA RAPIDS CMake package. Their system depends on a newer version of libcudacxx that needs to be evaluated after the system includes of CUDA, which contains an older version of libcudacxx. This is achieved by marking libcudacxx as INTERFACE_INCLUDE_DIRECTORIES (-I) and CUDA as INTERFACE_SYSTEM_INCLUDE_DIRECTORIES (-isystem). Since Meson ignores the SYSTEM flags, it just includes everything with -I and the conflict begins.

@dcbaker dcbaker added the module:cmake Issues related to the cmake module, including cmake.subproject label Aug 20, 2024
@dcbaker dcbaker changed the title Implement SYSTEM handler for cmake dependencies. Implement SYSTEM handler for cmake subprojects. Aug 20, 2024
@dcbaker
Copy link
Member

dcbaker commented Aug 20, 2024

Failures looks relevant. It would also be good to add something to one of the existing tests to cover this.

@luigifcruz
Copy link
Author

@dcbaker Are you sure about the title change? Looks like the handlers for dependency and subprojects are different. This PR adds support to import ***.cmake packages while #6199 adds support for subprojects.

@dcbaker
Copy link
Member

dcbaker commented Aug 20, 2024

You are right, I just looked at the traceparser and thought that was subprojects

@dcbaker dcbaker changed the title Implement SYSTEM handler for cmake subprojects. Implement SYSTEM handler for cmake dependencies. Aug 20, 2024
@dcbaker dcbaker added dependency:cmake Issues related to `dependency` with the `cmake` method and removed module:cmake Issues related to the cmake module, including cmake.subproject labels Aug 20, 2024
@eli-schwartz
Copy link
Member

Revert manual merge line deletion.

Please always use git commit --fixup followed by git rebase -i --autosquash (or just git commit --amend if it is the most recent commit) to fix issues in previous commits.

In general, each commit should represent a discrete change proposal or step in the process of achieving the goal of a PR.

@luigifcruz
Copy link
Author

@eli-schwartz Yes, I'll just check if this will pass CI and then cleanup for merge.

@luigifcruz
Copy link
Author

luigifcruz commented Aug 21, 2024

All tests appear to be passing now. I'll add a test for SYSTEM includes shortly.

@eli-schwartz
Copy link
Member

This doesn't include any tests for SYSTEM so all we actually know is that it doesn't regress currently tested scenarios.

Would it be possible to add some simple mockup of the original issue? e.g. one of the tests in test cases/cmake/ sets up dependency(xxx, method: 'cmake', cmake_module_path: 'cmake') to "Try to find a dependency with a custom CMake module". It doesn't have to be a full-blown built project, just whatever minimal interface it takes to to fail when -isystem is missing.

(Perhaps some trick with #include_next?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency:cmake Issues related to `dependency` with the `cmake` method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants