VCPKG bad include folder structure about polyhook_2_0 HOT 29 CLOSED

microsoft/vcpkg#9932

The pr has been merged, using vcpkg install polyhook2 should work as expected now!

from polyhook_2_0.

I was actually in the process of writing up my own issue for this very same problem.

There is a further issue, even if you correct the headers by removing the "header/" and "detour/" chunks, something is wrong with how capstone is being compiled.

It seems like capstone has no active architectures enabled

I used

capstone:x86-windows-static
polyhook2:x86-windows-static

setup a small program c++ program

added

 <VcpkgTriplet Condition="'$(Platform)'=='Win32'">x86-windows-static</VcpkgTriplet>

to the
 <PropertyGroup Label="Globals">
in the .vcxproj

#include "CapstoneDisassembler.hpp"
#include "x86Detour.hpp"

NOINLINE int __cdecl hookMe1() {
	volatile int var = 1;
	volatile int var2 = 0;
	var2 += 3;
	var2 = var + var2;
	var2 *= 30 / 3;
	var = 2;
	printf("%d %d\n", var, var2); // 2, 40
	return var;
}

uint64_t hookMe1Tramp = NULL;
NOINLINE int __cdecl h_hookMe1() {
	std::cout << "Hook 1 Called!" << std::endl;

	//effects.PeakEffect().trigger();
	return PLH::FnCast(hookMe1Tramp, &hookMe1)();
}

int main()
{
	PLH::CapstoneDisassembler dis(PLH::Mode::x86);
	PLH::x86Detour detour((char*)&hookMe1, (char*)&h_hookMe1, &hookMe1Tramp, dis);
	detour.hook();
}

and upon execution you get

error opening cap

I stepped into cs_open and it seems like cs_arch_init is empty. So even if the header issue gets resolved it seems like there is a secondary issue with capstone. I've not used vcpkg for very long either so perhaps I too am missing something

from polyhook_2_0.

Doing some further digging into how capstone is configured as its headers are inside the capstone directory.

Inside capstones cmake

install(FILES ${HEADERS_COMMON} DESTINATION include/capstone)

from the cmake_install.cmake for capstone

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/include/capstone" TYPE FILE FILES
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/arm64.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/arm.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/capstone.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/evm.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/mips.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/ppc.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/x86.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/sparc.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/systemz.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/xcore.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/m68k.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/tms320c64x.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/m680x.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/platform.h"
    )
endif()

seems like this is what vcpkg is using to decide where its output headers go.

Since this repo is lacking any such configuration everything gets dropped into the base includes directory which is not optimal.

I guess there are a few different ways this could be resolved, but I imagine the cleanest way would be to specify a overarching include directory for all the files so everything gets put inside a single polyhook2 directory, and then specify the relative folders for where every header is supposed to be so that cmake puts them in the correct place.

Otherwise a similar patch file that's inside the polyhook2 ports directory for vcpkg could be setup to modify all the headers, but I think having all of polyhooks headers just dropped into the base includes folder isn't very good style.

from polyhook_2_0.

Looks like the capstone issue was resolved with

vcpkg install capstone[x86]:x86-windows-static --recurse

to enable the x86 capstone feature.

Looks like the vcpkg for polyhook2 needs to have

Build-Depends: capstone
replace with
Build-Depends: capstone[x86]
as that configures capstone correctly.

from polyhook_2_0.

Looks like polyhook2 does have a headers install location, its just the very last line in the cmake

install(FILES ${HEADER_FILES} DESTINATION include)

from polyhook_2_0.

Hey guys sorry I do not use vcpkg myself and another contributer submitted it as a build. If you would like to submit a PR I would happily review, would definitely be interested in fixing.

from polyhook_2_0.

I don't use cmake very often so I'm not sure if theres a better way to do this.

I modified the existing cmake and added

install(FILES ${HEADER_FILES} DESTINATION include/headers)
install(FILES ${DETOUR_HEADER_FILES} DESTINATION include/headers/Detour)
install(FILES ${PE_HEADER_FILES} DESTINATION include/headers/PE)
install(FILES ${VIRTUAL_HEADER_FILES} DESTINATION include/headers/Virtuals)
install(FILES ${EXCEPTION_HEADER_FILES} DESTINATION include/headers/Exceptions)

to the bottom, this sets up all the headers into the correct directories, but again I don't think its great having everything just shoved into a random folder called headers.

We could rename the headers directory to polyhook2 and then modify the cmake to be

install(FILES ${HEADER_FILES} DESTINATION include/polyhook2 )
install(FILES ${DETOUR_HEADER_FILES} DESTINATION include/polyhook2/Detour)
install(FILES ${PE_HEADER_FILES} DESTINATION include/polyhook2/PE)
install(FILES ${VIRTUAL_HEADER_FILES} DESTINATION include/polyhook2/Virtuals)
install(FILES ${EXCEPTION_HEADER_FILES} DESTINATION include/polyhook2/Exceptions)

that would create a very nice directory structure, but we would have to modify all the existing files in the repo that include

#include "headers/..."

@stevemk14ebr what do you think about this solution?

Regardless we also will have to submit a pr to the vcpkg repo to enable capstone[x86] but I don't think that will be too big of a deal, but should we add some of the other capstone features too, or is x86 the only ones used by polyhook2?

from polyhook_2_0.

I have no problem with relative imports. If you submit a PR with all the required changes I can review and test the building and then we can tackle vcpkg. I got lots of tests, so should be easily to tell if it gets messed up

from polyhook_2_0.

Alright, I have to go out for a few hours when I get back I'll try issing a pr.

from polyhook_2_0.

This looks like it may help instead, maybe try this? https://stackoverflow.com/questions/11096471/how-can-i-install-a-hierarchy-of-files-using-cmake

and yes we only use the x86/x64 feature of capstone. Polyhook cannot hook any other architectures.

from polyhook_2_0.

That works as well. I was just looking at the patchfile included in the vcpkg repo

https://github.com/microsoft/vcpkg/blob/master/ports/polyhook2/fix-build-error.patch

and noticed that it was what added the install command, so the question is should the replacement command be merged into the main repo as well, or should it merely be updated inside that patch.

I've issued #51 which has ommited the change, if you think it should be included then I'll push another commit that adds

INSTALL(DIRECTORY ${PROJECT_SOURCE_DIR}/polyhook2 DESTINATION include/) which worked when i tested it

from polyhook_2_0.

I am fine with the changes in that patch file, if you wouldn't mind adding them. Please verify that it builds locally without vcpkg as well, and then if this all works ill merge and we can move onto the vcpkg PR

from polyhook_2_0.

Including those changes breaks compiling outside vcpkg

from polyhook_2_0.

1> [CMake] CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
1> [CMake] Please set them or make sure they are set and tested correctly in the CMake files:
1> [CMake] C:/Users/x/Documents/PolyHook_2_0/CAPSTONE_INCLUDE_DIR
1> [CMake]    used as include directory in directory C:/Users/x/Documents/PolyHook_2_0
1> [CMake] CAPSTONE_LIBRARY
1> [CMake]     linked by target "PolyHook_2" in directory C:/Users/x/Documents/PolyHook_2_0

from polyhook_2_0.

Could leave this portion in, as there was no issue compiling inside visual studio with it

#Install targets
install(TARGETS ${PROJECT_NAME}
  RUNTIME DESTINATION bin
  LIBRARY DESTINATION lib
  ARCHIVE DESTINATION lib
)

#Install headers
INSTALL(DIRECTORY ${PROJECT_SOURCE_DIR}/polyhook2 DESTINATION include/)

from polyhook_2_0.

I've gone and pushed another commit to that pr that adds that second chunk

from polyhook_2_0.

tested and merged, thanks. Please author the appropriate PR for vcpkg now

from polyhook_2_0.

I've incorporated these changes into the zydis branch as well. If it is possible please allow vcpkg to build with that branch too. Your work is appreciated very much.

from polyhook_2_0.

I've only been using vcpkg for a few days, when I wanted to add gumbo to my new project (https://github.com/xeropresence/AOPP) I wanted to make it easy for others to compile so that's when I started looking into it and noticed polyhook2 (which is also used by my new project) was on there as well. With that being said, im not sure how you would separate capstone vs zydis, perhaps as a 'feature'? I'll look into getting a PR into the vcpkg repo, looks like I'll have to sign and agree to their CLA.

from polyhook_2_0.

If it's too hard no worries 😃

from polyhook_2_0.

ok i made it easy. Zydis was a super set of capstone, so now master has both capstone and zydis in it and i deleted the branch.

from polyhook_2_0.

theres nothing preventing someone from compiling with both zydis and capstone correct? you would decide which one you wanted when you passed in the dissam to the hook right?

from polyhook_2_0.

That's correct. They're fully abstracted see the templated tests:

from polyhook_2_0.

Looks like that actually makes it more complicated.

vcpkg now fails even with setting DEP_ZYDIS_NEED to off.

The project is including CapstoneDisassembler.hpp and ZydisDisassembler.hpp regardless of the flags, and as such has a hard-dependency on them.

from polyhook_2_0.

When I get a chance I'll make the includes guarded. Make it without those set to off for now and I'll fix it up.

from polyhook_2_0.

I've got it so locally vcpkg will compile it.

I was looking over the other patch they include

https://github.com/microsoft/vcpkg/blob/master/ports/polyhook2/fix-build-tests-error.patch

and this made me wonder if the default build config for the project is optimal.

Would it make more sense for the default build configuration be to construct the library/dll instead of the unit tests?

from polyhook_2_0.

Looks like zydis compat is out the window for the moment anyway,

microsoft/vcpkg#8426

they are still using version 2.0 which lacks zycore

from polyhook_2_0.

is it possible to build zydis without using the version in vcpkg? For example i don't believe we use the capstone version that is in vcpkg (if there even is one)

from polyhook_2_0.

vcpkg uses the capstone vcpkg. I've issued a vcpkg pr here microsoft/vcpkg#9932

from polyhook_2_0.