Skip to content

Add cmake option to tell whether architectures should be included by default#1463

Merged
aquynh merged 9 commits intocapstone-engine:masterfrom
JornVernee:arch_default
Apr 29, 2019
Merged

Add cmake option to tell whether architectures should be included by default#1463
aquynh merged 9 commits intocapstone-engine:masterfrom
JornVernee:arch_default

Conversation

@JornVernee
Copy link
Copy Markdown
Contributor

@JornVernee JornVernee commented Apr 23, 2019

I'm statically linking capstone, and my compiler is Visual Studio. But, some of the architecture specific code relies on gcc extensions it seems, so the linking fails. As a result I have to disable these architectures, but really I only need x86 support any ways.

It would be nice if the default for including architectures could be configured, so I'd only have to pass 2 flags, e.g.

-DCAPSTONE_ARCHITECTURE_DEFAULT=OFF -DCAPSTONE_X86_SUPPORT=ON

Instead of having to explicitly disable all the other architectures.

So, that's what this PR does.

@JornVernee JornVernee changed the title Add cmake option to tell whether an architecture should be included by default Add cmake option to tell whether architectures should be included by default Apr 23, 2019
@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 28, 2019

but we already had this: did you look at https://github.com/aquynh/capstone/blob/master/cmake-x86.sh?

@JornVernee
Copy link
Copy Markdown
Contributor Author

@aquynh Oh, I totally missed this, sorry.

I didn't look for anything x86 specific in the CMakeLists file, just at what the default was :) I guess this is only supported for x86 specifically?

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 28, 2019 via email

…use the CAPSTONE_ARCHITECUTRE_DEFAULT option, which can be used with any architecture.
@JornVernee
Copy link
Copy Markdown
Contributor Author

JornVernee commented Apr 28, 2019

How is this? I removed the CAPSTONE_X86_ONLY flag and updated the build scripts to use the CAPSTONE_ARCHITECTURE_DEFAULT flag instead.

This should work equally for other architectures as well, and allows building multiple architectures without having to disable all others first. But, I haven't added shell scripts for every one of them (seems a bit excessive?)

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 28, 2019

we can modify cmake.sh, and build architecture as option (so that if nothing is passed in, we compile all)?

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 28, 2019

we also have https://github.com/aquynh/capstone/blob/master/nmake-x86.bat (and nmake.bat) for Windows, that should be changed too.

@JornVernee
Copy link
Copy Markdown
Contributor Author

we can modify cmake.sh, and build architecture as option (so that if nothing is passed in, we compile all)?

I don't see any cmake.sh though? And yes, the option still builds all architectures by default, see: diff. The default is ON.

we also have https://github.com/aquynh/capstone/blob/master/nmake-x86.bat (and nmake.bat) for Windows, that should be changed too.

I've updated nmake-x86.bat already :)

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 28, 2019

yes, then you can create cmake.sh.

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 28, 2019

i think we can make nmake.sh to do similar thing (that is to support building individual archs), then remove nmake-x86.sh & cmake-x86.sh.

@JornVernee
Copy link
Copy Markdown
Contributor Author

JornVernee commented Apr 28, 2019

Ok. I've updated those scripts and removed nmake-x86.bat & cmake-x86.sh. Now you can do e.g.

../cmake.sh x86

For x86, etc.

I was not able to fully test nmake.bat though. I'm also on Windows, but I just invoke cmake directly with the default Visual Studio generator.

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 29, 2019

can you please update https://github.com/aquynh/capstone/blob/master/COMPILE_CMAKE.TXT with these new scripts?

@JornVernee
Copy link
Copy Markdown
Contributor Author

JornVernee commented Apr 29, 2019

I've updated the documentation w.r.t. this PR, and added some more instructions as well, like how to use the install target, or how to pass configuration flags, and build directly through cmake (like I'm doing).

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 29, 2019

very nice, can you update the section about nmake.bat too (like nmake.bat x86 can be used to build X86 only lib)?

@JornVernee
Copy link
Copy Markdown
Contributor Author

JornVernee commented Apr 29, 2019

@aquynh I've put that at (3), together with a list of all the supported architectures. I mention that the option works for either cmake.sh or nmake.bat. Is that ok?

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 29, 2019

ah i see, that is ok then.

@aquynh aquynh merged commit e5d7120 into capstone-engine:master Apr 29, 2019
@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 29, 2019

merged, thanks!

@JornVernee
Copy link
Copy Markdown
Contributor Author

Great 😄

@aquynh
Copy link
Copy Markdown
Collaborator

aquynh commented Apr 29, 2019

would you please make similar pull req for "next" & "v4" branches? these branches either have or do not have some architectures, so i cannot cherry pick this pull req.

@JornVernee
Copy link
Copy Markdown
Contributor Author

Sure, I'll take a look

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants