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

Have dolphin_scmrev run ScmRevGen from CMAKE_CURRENT_SOURCE_DIR #12591

Merged
merged 2 commits into from Feb 20, 2024

Conversation

JesseTG
Copy link
Contributor

@JesseTG JesseTG commented Feb 19, 2024

This is another little fix to simplify FetchContent'ing Dolphin. Without this PR, the dolphin_scmrev target will try to use a file named ScmRevGen.cmake in the including project's CMake directory.

@AdmiralCurtiss
Copy link
Contributor

Makes sense. There's a few other instances of CMAKE_SOURCE_DIR that probably want to execute relative to the current directory instead of relative of the top-level CMake file, too, such as

dolphin/CMakeLists.txt

Lines 158 to 160 in acb18a5

list(APPEND CMAKE_MODULE_PATH
${CMAKE_SOURCE_DIR}/CMake
)

@JesseTG
Copy link
Contributor Author

JesseTG commented Feb 19, 2024

Makes sense. There's a few other instances of CMAKE_SOURCE_DIR that probably want to execute relative to the current directory instead of relative of the top-level CMake file, too, such as

dolphin/CMakeLists.txt

Lines 158 to 160 in acb18a5

list(APPEND CMAKE_MODULE_PATH
${CMAKE_SOURCE_DIR}/CMake
)

Sure, I can knock 'em out for you if you'd like. I'll comb through myself, but are there any instances you'd like to highlight?

@JesseTG
Copy link
Contributor Author

JesseTG commented Feb 19, 2024

@AdmiralCurtiss How's this?

@AdmiralCurtiss
Copy link
Contributor

Err, appreciated but I don't think that's correct. CMAKE_CURRENT_SOURCE_DIR gives you directory of the currently running CMakeLists.txt, so for most of these you have to go up a few levels in order to end up at the same relative location, eg.:

@AdmiralCurtiss
Copy link
Contributor

I'm guessing you will be back but yeah sure this is fine.

@AdmiralCurtiss AdmiralCurtiss merged commit 3a41d99 into dolphin-emu:master Feb 20, 2024
11 checks passed
@JesseTG
Copy link
Contributor Author

JesseTG commented Feb 20, 2024

I'm guessing you will be back but yeah sure this is fine.

Yes, this is a safe bet. Thanks for merging this. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants