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

Externals: Eliminate the need to have the root "Externals" directory as an include path. #8383

Open
wants to merge 1 commit into
base: master
from

Conversation

@jordan-woyak
Copy link
Member

jordan-woyak commented Oct 5, 2019

The primary CMakeLists.txt made it look like include_directories(Externals) was just providing soundtouch headers but it's really pulling in way more.

I've moved a few "Externals" headers around so providing their includes isn't so horrible.

@jordan-woyak jordan-woyak force-pushed the jordan-woyak:externals-include-path branch 3 times, most recently from 6e3f077 to 6edaed9 Oct 5, 2019
…as an include path.
@jordan-woyak jordan-woyak force-pushed the jordan-woyak:externals-include-path branch from 6edaed9 to f29ad78 Oct 5, 2019
@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Oct 5, 2019

Those now double Externals/ed25519/ed25519, Externals/soundtouch/soundtouch etc. look a bit weird; is this really necessary? I don't speak too much CMake, so I can't really tell...

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Oct 5, 2019

Being primarily a windows user, I can't speak to the CMake logic in Dolphin. However, I use quite a bit of CMake in other places. In "modern" CMake, include_directories in of itself is technically bad practice (since it is a global include). I'd probably make all the externals individual targets that source in their include directories and drop the global include altogether.

As for this change, the only thing I don't personally care for is the way the discord include now works (from #include <discord-rpc/include/discord_rpc.h> to #include <discord_rpc.h>) but that's mostly down to personal preference. Otherwise, this seems fine.

@jordan-woyak

This comment has been minimized.

Copy link
Member Author

jordan-woyak commented Oct 5, 2019

Those now double Externals/ed25519/ed25519, Externals/soundtouch/soundtouch etc. look a bit weird; is this really necessary? I don't speak too much CMake, so I can't really tell...

It looks weird to me too. I'm open to suggestions on better ways to do it. :P

Being primarily a windows user, I can't speak to the CMake logic in Dolphin. However, I use quite a bit of CMake in other places. In "modern" CMake, include_directories in of itself is technically bad practice (since it is a global include). I'd probably make all the externals individual targets that source in their include directories and drop the global include altogether.

As for this change, the only thing I don't personally care for is the way the discord include now works (from #include <discord-rpc/include/discord_rpc.h> to #include <discord_rpc.h>) but that's mostly down to personal preference. Otherwise, this seems fine.

I don't think discord-rpc/include/discord_rpc.h is normal/correct. Other projects appear to use just discord_rpc.h.

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Oct 5, 2019

I don't think discord-rpc/include/discord_rpc.h is normal/correct. Other projects appear to use just discord_rpc.h.

Yeah, I can't speak to the discord include itself. I was more commenting on the style of include looking odd to me but that probably depends on the team. Personally, I'm used to seeing <libraryname>/<header>. However, if groups using discord use it like #include <discord_rpc.h> then I agree we should follow that trend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.