search

zyantific / zydis

Fast and lightweight x86/x86-64 disassembler and code generation library
https://zydis.re
MIT License
3.4k stars 434 forks source link

Check if Zydis is the top-level project for better FetchContent support #459

Closed ZehMatt closed 11 months ago

ZehMatt commented 11 months ago

This prevents targets such as examples, doxygen, etc. from being added when consumed via FetchContent

athre0z commented 11 months ago

Hmm, not a fan of removing the options entirely if ZYDIS_ROOT_PROJECT isn't set. This will probably break vcpkgs build which itself uses CMake to describe package builds (so Zydis won't be top-level project).

I'd simply change the default values of those properties that are currently ON to ${ZYDIS_ROOT_PROJECT} instead. This sidesteps the issue and should provide the same desired outcome.

flobernd commented 11 months ago

I suggested doing it this way to as well reduce option pollution while we are already on it.

Not sure about vcpkg, but that needs to be checked first. I'm fine with your approach as well, if removing the options might break some stuff.

Cmake itself is pretty forgiving with the options. Not declared ones are simply assumed to be OFF.

ZehMatt commented 11 months ago

I've defaulted some of the options that were ON by default to ${ZYDIS_ROOT_PROJECT}, this gives us the same result but keeps the options alive and one can still override them with cmake command line using -D, so I think this is the most optimal approach.

athre0z commented 11 months ago

I suggested doing it this way to as well reduce option pollution while we are already on it.

I don't think that our options are excessive. I far prefer having all the options visible over the user having to figure out why the hell some options magically disappeared once they include Zydis into their project.

flobernd commented 11 months ago

Fine for me!

Maybe let's do the same change in Zycore and update the submodule as part of this PR before merging?

athre0z commented 11 months ago

Zycore doesn't appear to have any default-ON options, so there shouldn't be any change required there. Or would you like to default-enable the tests there with a similar switch? That'd be fine with me.

flobernd commented 11 months ago

Yeah it's inconsistent at the moment. Would be better to have the same logic in Zycore as well. But as the tests are currently OFF for Zycore, we can as well merge right away 🙂