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

CMake cleanup #1489

Open
jpc0 opened this issue Jun 1, 2023 · 5 comments
Open

CMake cleanup #1489

jpc0 opened this issue Jun 1, 2023 · 5 comments

Comments

@jpc0
Copy link
Contributor

jpc0 commented Jun 1, 2023

This would just a thread to discus what needs to be cleaned up in the cmake configs and what direction we would want to go with some things. Pinging @dimitry-ishenko in this since he may be able to assist in the near future.

Some ideas:

  1. Move PCH from custom implementation to CMake native implementation
  2. Seperate out public and private includes in cmake
    • Currently all the includes are using the older include_directories command
    • It would be beneficial to have the public headers used by other targets exported as public and those that are implementation detail exported as private
  3. Fix windows and linux build process being dramatically different
  4. Cmake install targets?
@Julusian
Copy link
Member

Julusian commented Jun 1, 2023

For 3, I have been looking at vcpkg in a branch. I'm not convinced on it yet, because it has the cost of making compile times much longer, unless I can find a good way to do binary caching (vcpkg does support this, but there isnt a good easy and free option).

An overview of the packages we use for each of vcpkg and nuget:
For the 9 dependencies we have, 3 are committed to the repository, 3 are published by me, 1 is up to date, 2 are a bit stale and look abandoned.
Switching to vcpkg, 8 of those are in the repository and maintained. The last, we can use a custom port. Realistically to make compiling bearable, ffmpeg may need to be avoided in vcpkg (compiling that in github actions takes 2+ hours, and it runs out of disk space), but we could use a custom port to provide a precompiled version. Compiling the other 8 dependencies takes about 20 minutes in github actions without caching.

@jpc0
Copy link
Contributor Author

jpc0 commented Jun 1, 2023

Yeah I am aware of the painful recompiles of ffmpeg, have a project on RPI where I need to do so and generally need half a day, boost isn't much better though if you need all of boost.

I don't think NuGet is necessarily bad, will just need some config in cmake but there is stuff that can be cleaned up and generalized.

@jpc0
Copy link
Contributor Author

jpc0 commented Jun 5, 2023

@dimitry-ishenko I'm continuing this here since it is relevant.

@jpc0 from your Core Guidelines link (emphasis mine):

Nevertheless, the guidance is to use the quoted form for including files that exist at a relative path to the file containing the #include statement (from within the same component or project) and to use the angle bracket form everywhere else, where possible.

In other words, files from your project, which are all located at some path relative to the current file, should use quotes. All others, meaning system files and other libs not part of your project, should use angle brackets.
They are saying the same thing the gcc link is saying.

I think the relevant section here is "from within the same component or project", we should be treating every one of the targets as a separate component with a public API that is exported and everything else being private internally. That way we could for instance in the future switch the current accelerator to WebGPU or OpenCL or whatever else (maybe something per platform) and not need to change code in 100 places.

@Julusian
Copy link
Member

Julusian commented Nov 1, 2023

3 is affected by #1502, now using cmake external packages instead of nuget (and caching in docker images?).
It doesn't clean anything up just yet, but does simplify it a bit by removing nuget. however, there is still a difference of using external packages on windows and system dependencies on linux so it doesnt really affect the cleanup all that much

@Julusian
Copy link
Member

Julusian commented Nov 6, 2023

For 3, I have done some further cleanup of the platform specific cmake files in #1504 to remove anything which was duplicated across them both. This makes them a lot easier to understand and follow the differences, it is mostly now a difference in compiler flags and approach to finding dependencies. Maybe more could be done to make this even better, but I am satisfied now that it is more understandable.

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

No branches or pull requests

2 participants