microsoft
[WIP] Feature: Adding CMake support
Try to add a simple CMakeLists.txt for building detours in x86/x64. Support for other architectures like ARM/ARM64/IA64 may be added later.
Since the uimports.cpp file is included in creatwth.cpp#L347, I suggest that the uimports.cpp file should be renamed to uimports.h, then we can use *.cpp pattern to add all source files in the CMakeLists.txt.
CMakeLists.txt for samples are on the way.
OK, including .cpp files happens many times in the samples. Will this be changed in the future?
It looks like Detours relies on build order of sources to compile reliable (AFAIS detours.cpp should be first). However, this is pretty bad as VS doesn't guarantee build order of source files.
I assume this is the reason your CMake file doesn't work for me. I guess your CMake configuration works for just one specific build order which I'm not getting on my machine. You are defining a lot of stuff which shouldn't be defined at all (e.g. DETOURS_VERSION, _X86_, DETOURS_X86, etc.).
Please note that the nmake Makefile is only passing /DWIN32_LEAN_AND_MEAN /D_WIN32_WINNT=0x501 to the compiler. (DETOURS_BITS=X is passed to rc.exe, but this is only relevant on building the examples).
My own CMake configuration is working with these two definitions, too. I have put detours.cpp at first entry in the source list. However, I'm not sure if this is reproducible or just luck.
I guess the correct approach would be to refactor Detours so it isn't dependent on build order anymore.
I can't agree more! My approach is now just for forcing to make specific platform work, to meet my basic need. I have been trying to do some research about the cross platform/architecture problem these days. We need a more uniform design.
@longhronshen, what is the status of this change? We do need a cmake configuration to work for different developers. Are there dependencies missing from the cmake file that would better express the build order?
Now the PR has been done for the main project, and several other example projects. What is still blocked is some example projects which involves some COM interops. If this is not the key problem, I can update the PR and then I think we can merge the PR.
I think it is fine to exclude some samples for now from the cmake build. We can merge the PR and that can be worked on later. Please make sure it is documented that those samples don't have a cmake build yet.
CMake support
CMake support for the main project has been done for a while, but some sample projects are excluded for some interop reasons.
- cping
- disas
- dtest
- dumpe
- dumpi
- echo
- einst
- excep
- findfunc
- impmunge
- member
- opengl
- slept
- talloc
- tryman
Other TODOs:
- ARM/ARM64/IA64 support
- Improve Sample project debugging experiences
I think the current version is ready for review, except for these excluded sample projects.
@LonghronShen can you enable "Allow edits from Maintainer" for this PR? See: https://github.blog/2016-09-07-improving-collaboration-with-forks/
I have some fixes / additions locally that I would like to include before we merge this PR.
Thanks!
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days.
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days.
Yea it's on my backlog of things to do.
Since the author doesn't seem available, I'm planning on cherry-picking their commits and then fixing them up, and pushing it once it's ready. Let me know if you are able / willing to help with that.
Preferably we should use a single build system for the entire project, because maintaining several is a maintenance burden more than anything else. Since CMake can generate Visual Studio solutions, maybe ditch nmake and the .sln?
Preferably we should use a single build system for the entire project, because maintaining several is a maintenance burden more than anything else. Since CMake can generate Visual Studio solutions, maybe ditch nmake and the .sln?
Yup, completely agree, and that's why I haven't merged this yet. I want it to be feature complete before we merge and use it the one true build system. CMake can generate NMake as well, so there will be no reason to keep the nmake or sln after this is merged.
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days.