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

miniscule fix for building on systems where the hardcoded path to libdrm headers doesn't exist #3387

Merged
merged 2 commits into from
May 28, 2024

Conversation

pillowtrucker
Copy link
Contributor

, e.g. NixOS.
I hope this still works on other systems.

@pillowtrucker pillowtrucker requested a review from a team as a code owner May 10, 2024 20:56
Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.52%. Comparing base (a5f3143) to head (be9edd1).

Current head be9edd1 differs from pull request most recent head 9b9393e

Please upload reports for the commit 9b9393e to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3387      +/-   ##
==========================================
- Coverage   77.52%   77.52%   -0.01%     
==========================================
  Files        1065     1065              
  Lines       67901    67901              
==========================================
- Hits        52639    52638       -1     
- Misses      15262    15263       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RAOF
RAOF previously requested changes May 11, 2024
Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

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

Neat!

This information should be available in pkgconfig, right? I think it would be a better idea to pull it from there (as that's where the compiler will pull it from) - I'm not sure if CMake's find_path will match the pkgconfig behaviour, and things could get weird if those don't match.

@pillowtrucker
Copy link
Contributor Author

Oh, I totally missed that there is a similar variable already set by pkg_check_modules.
The problem with that though is that it can (and does) return more than one directory, so it would still have to be something like

find_path(DRM_FOURCC_INCLUDE_DIR NAMES "drm_fourcc.h" PATH_SUFFIXES "libdrm" "" HINTS ${DRM_INCLUDE_DIRS})

which is kind of redundant, but I guess it doesn't hurt either. find_path always returns the first match, so it's guaranteed to be at most one entry. Should I make that change ?

@pillowtrucker pillowtrucker force-pushed the fix-build-without-fhs branch from 3a28937 to be9edd1 Compare May 27, 2024 14:15
@pillowtrucker
Copy link
Contributor Author

Fixed up and rebuilt and briefly tested. There was a third occurrence of the hardcoded path there that I had missed which apparently didn't cause any build or runtime failure on my computer, but left the DRM_MODIFIERS_FILE incomplete, that's also been taken into account now.

@pillowtrucker pillowtrucker requested a review from RAOF May 27, 2024 14:22
@RAOF
Copy link
Contributor

RAOF commented May 28, 2024

Thanks for the ping! This fell off my plate when travelling to the recent sprint, the week-long sprint, and then the travel back 😉

I've tweaked the find_path invocation slightly, to make it fail if it doesn't find drm_fourcc.h and to make sure it only looks in places pkgconf lists. Please check that this still works for you, and then I think this is good!

@pillowtrucker
Copy link
Contributor Author

Yep that still builds and runs on nix. Good call, because otherwise the find_* functions first look in all of the paths in one of the CMAKE_* environment variables set by nix.

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Ignoring paranoid thoughts like "what if DRM_FOURCC_INCLUDE_DIR includes spaces?" this looks reasonable.

@AlanGriffiths AlanGriffiths added this pull request to the merge queue May 28, 2024
Merged via the queue into canonical:main with commit a6ec106 May 28, 2024
21 of 23 checks passed
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