[WIP] Feature: Adding CMake support #48

LonghronShen · 2018-12-10T15:14:28Z

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.

msftclas · 2018-12-10T15:14:39Z

All CLA requirements met.

dtarditi · 2019-01-23T21:14:28Z

CMakeLists.txt

+ENDIF()
+
+FILE(GLOB detours_src
+    ${CMAKE_CURRENT_LIST_DIR}/src/*.h


Could you alphabetize the list of files here? I believe some source files are missing from the list. Also, please list out the header files - don't use the *.h pattern.

Thank you for your suggestions!
Since I'm not quite familiar with the Detours itself, I just use the script then it builded successfully. I will sort all these files here.
Then you're right, it is not nesscessary to add the header files. I'll remove the pattern, and use INCLUDE_DIRECTORIES instead.

dtarditi · 2019-01-23T21:15:29Z

Thanks for the proposed change. I have some suggestions.

LonghronShen · 2019-01-26T14:39:03Z

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?

schlamar · 2019-02-04T10:18:07Z

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.

LonghronShen · 2019-02-04T10:32:54Z

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.

dtarditi · 2019-05-15T22:24:35Z

@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?

LonghronShen · 2019-05-16T09:45:01Z

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.

dtarditi · 2019-05-16T18:27:51Z

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.

LonghronShen · 2019-05-16T18:42:11Z

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

dtarditi · 2019-05-16T20:11:05Z

Please let me know when this is ready for further review.

LonghronShen · 2019-05-17T02:09:38Z

I think the current version is ready for review, except for these excluded sample projects.

bgianfo · 2020-09-02T06:29:34Z

CMakeLists.txt

+	LIST(APPEND DETOURS_COMPILE_DEFINITIONS "DETOURS_TARGET_PROCESSOR=X64")
+	LIST(APPEND DETOURS_COMPILE_DEFINITIONS DETOURS_X64)
+	LIST(APPEND DETOURS_COMPILE_DEFINITIONS DETOURS_64BIT)
+	LIST(APPEND DETOURS_COMPILE_DEFINITIONS _AMD64_)


The MSVC compiler sets these based on the architecture the configured compiler is targeting, the build system shouldn't be setting it them selves.

This is just imitating the actions written in Makefiles. Or can we change the code so that we can use modern ways to check these staffs?

bgianfo · 2020-09-02T06:30:49Z

cmake/FindCSharp.cmake

+##
+
+## This file is based on the work of SimpleITK:
+##  https://github.com/SimpleITK/SimpleITK/blob/master/CMake/FindCSharp.cmake


We can't copy and paste code from other projects into Detours, this code has it's own LICENSE.

For this, I'll see if we can solve the problem by referencing the third party staffs as submodule or some other ways...

CMake 3.8+ has native support for C#
See: https://cmake.org/cmake/help/latest/release/3.8.html#id4

bgianfo · 2020-09-02T06:31:29Z

CMakeLists.txt

@@ -0,0 +1,79 @@
+CMAKE_MINIMUM_REQUIRED(VERSION 3.10)


Why is everything in all caps? can we fix it to be lower case?

As for older cmake code style, commands are written in UPPER CASE. Now it is year 2020, I'll change this.

I can run https://github.com/cheshirekow/cmake_format on the files if I can get push access to this PR.

bgianfo · 2020-09-02T07:06:32Z

Can you fix the merge conflicts in .gitignore?

LonghronShen · 2020-09-02T07:14:40Z

OK, I'll take it in the next one or two days.

bgianfo · 2021-01-24T10:11:02Z

@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!

ghost · 2021-01-31T17:00:26Z

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.

ghost · 2021-03-10T00:34:35Z

Any update on this?

ghost · 2021-03-17T05:00:22Z

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.

tostercx · 2021-09-11T18:28:34Z

Any chance of this getting merged?

bgianfo · 2021-09-13T17:48:59Z

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.

sylveon · 2021-09-13T18:22:16Z

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?

bgianfo · 2021-09-13T18:28:49Z

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.

ghost · 2021-09-20T23:00:50Z

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.

Add CMakeLists.txt

7e398ed

LonghronShen mentioned this pull request Dec 10, 2018

Added vc sln and vcxproj files #26

Closed

dtarditi reviewed Jan 23, 2019

View reviewed changes

sort source file list in the CMakeLists.txt and add a .gitignore file.

f182a9c

LonghronShen changed the title ~~Add CMake support~~ [WIP] Adding CMake support Jan 26, 2019

LonghronShen added 3 commits May 17, 2019 02:32

partial done for samples

91d8a31

exclude some sample projects for now

c6c57c9

Add documents for the CMake support.

4ff43ef

fix some typos

7525884

bgianfo requested a review from dtarditi August 17, 2020 17:58

bgianfo added the enhancement This adds new functionallity to the product label Aug 21, 2020

sylveon mentioned this pull request Aug 27, 2020

fix many bugs found by me, and reports by others in official repo. #127

Closed

bgianfo mentioned this pull request Aug 28, 2020

Ununderstandable documents (wiki) #49

Open

bgianfo reviewed Sep 2, 2020

View reviewed changes

Merge branch 'master' into master

510d11b

bgianfo mentioned this pull request Sep 7, 2020

add vs solution, it is safety because it only call nmake command, and it is friendly for edit, modify and debug code in Detours #138

Merged

Merge branch 'master' into master

7d5de4f

bgianfo added the needs-author-feedback This issue / or pull request requires author feedback before more action can be taken. label Jan 24, 2021

ghost added the status-no recent activity label Jan 31, 2021

bgianfo changed the title ~~[WIP] Adding CMake support~~ [WIP] Feature: Adding CMake support Mar 5, 2021

ghost removed the status-no recent activity label Mar 5, 2021

ghost added the status-no recent activity label Mar 17, 2021

ghost removed the status-no recent activity label Sep 11, 2021

ghost added the status-no recent activity label Sep 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feature: Adding CMake support #48

[WIP] Feature: Adding CMake support #48

LonghronShen commented Dec 10, 2018

msftclas commented Dec 10, 2018 •

edited

Loading

dtarditi Jan 23, 2019

LonghronShen Jan 26, 2019 •

edited

Loading

dtarditi commented Jan 23, 2019

LonghronShen commented Jan 26, 2019 •

edited

Loading

schlamar commented Feb 4, 2019 •

edited

Loading

LonghronShen commented Feb 4, 2019 •

edited

Loading

dtarditi commented May 15, 2019

LonghronShen commented May 16, 2019

dtarditi commented May 16, 2019

LonghronShen commented May 16, 2019 •

edited

Loading

dtarditi commented May 16, 2019

LonghronShen commented May 17, 2019

bgianfo Sep 2, 2020

LonghronShen Sep 2, 2020

bgianfo Sep 2, 2020

LonghronShen Sep 2, 2020

bgianfo Jan 24, 2021

bgianfo Sep 2, 2020

LonghronShen Sep 2, 2020 •

edited

Loading

bgianfo Jan 24, 2021

bgianfo commented Sep 2, 2020

LonghronShen commented Sep 2, 2020

bgianfo commented Jan 24, 2021

ghost commented Jan 31, 2021

ghost commented Mar 10, 2021

ghost commented Mar 17, 2021

tostercx commented Sep 11, 2021

bgianfo commented Sep 13, 2021

sylveon commented Sep 13, 2021

bgianfo commented Sep 13, 2021

ghost commented Sep 20, 2021

[WIP] Feature: Adding CMake support #48

Are you sure you want to change the base?

[WIP] Feature: Adding CMake support #48

Conversation

LonghronShen commented Dec 10, 2018

msftclas commented Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

LonghronShen Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

dtarditi commented Jan 23, 2019

LonghronShen commented Jan 26, 2019 • edited Loading

schlamar commented Feb 4, 2019 • edited Loading

LonghronShen commented Feb 4, 2019 • edited Loading

dtarditi commented May 15, 2019

LonghronShen commented May 16, 2019

dtarditi commented May 16, 2019

LonghronShen commented May 16, 2019 • edited Loading

CMake support

dtarditi commented May 16, 2019

LonghronShen commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LonghronShen Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgianfo commented Sep 2, 2020

LonghronShen commented Sep 2, 2020

bgianfo commented Jan 24, 2021

ghost commented Jan 31, 2021

ghost commented Mar 10, 2021

ghost commented Mar 17, 2021

tostercx commented Sep 11, 2021

bgianfo commented Sep 13, 2021

sylveon commented Sep 13, 2021

bgianfo commented Sep 13, 2021

ghost commented Sep 20, 2021

msftclas commented Dec 10, 2018 •

edited

Loading

LonghronShen Jan 26, 2019 •

edited

Loading

LonghronShen commented Jan 26, 2019 •

edited

Loading

schlamar commented Feb 4, 2019 •

edited

Loading

LonghronShen commented Feb 4, 2019 •

edited

Loading

LonghronShen commented May 16, 2019 •

edited

Loading

LonghronShen Sep 2, 2020 •

edited

Loading