Skip to content

Rust binding: call cmake in build script #1444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

Hakuyume
Copy link

To use unicorn via crates.io or git dependency, libunicorn.a should be built in build.rs.

Copy link

@Lani698 Lani698 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request

Copy link

@makotoshimazu makotoshimazu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I want this too.
(Btw I found anyone can press the approve button. I'm wondering if it's working as intended. I want to withdraw the approval and remove this comment but didn't find a way...)

@wtdcode
Copy link
Member

wtdcode commented Oct 3, 2021

Link to #1217 #1449

@aquynh
Copy link
Member

aquynh commented Oct 3, 2021 via email

@wtdcode wtdcode added this to the Unicorn2 Official Release milestone Oct 5, 2021
@Hakuyume Hakuyume force-pushed the build-in-build-script branch from 2315bdd to e785c11 Compare October 6, 2021 13:43
@Hakuyume Hakuyume changed the base branch from next to dev October 6, 2021 13:43
@Hakuyume
Copy link
Author

Hakuyume commented Oct 6, 2021

please submit pull req to add Rust binding to the "dev" branch of Unicorn2.

OK. I rebased this PR on the dev branch

@Hakuyume
Copy link
Author

Hakuyume commented Oct 6, 2021

My build script ignores binary that built in previous cargo build.
I will investigate it.

@bet4it
Copy link
Contributor

bet4it commented Oct 6, 2021

Could you reuse the library if it's already installed on the system? We can just do a dynamic link on it.

Originally posted by @bet4it in #1447 (comment)

@Hakuyume
Copy link
Author

@bet4it

I updated the build script to check LIBUNICORN environment variable.
If the variable is set, the build script uses it instead of building own static library.
You can specify both system-wide and local dynamic library via the variable.

Since I think the behavior of build script should be controlled explicitly, I omitted automatic fallback.

@bet4it
Copy link
Contributor

bet4it commented Oct 14, 2021

We could set it as a feature?
This is how keystone does this: https://github.com/keystone-engine/keystone/blob/master/bindings/rust/keystone-sys/build.rs

@Hakuyume
Copy link
Author

We could set it as a feature?

OK
I added two features: build and system.
build calls cmake and links the built binary statically.
system calls pkg-config and links the system binary.

);
link_lib(Some(build_helper::LibKind::Static), "unicorn");
println!("cargo:rustc-link-lib=static=unicorn");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not enough unfortunately, you would get linkage error on some systems. See https://github.com/wtdcode/unicorn-rs/blob/05891bd3e5710b26e871699e86ed3bc35028f630/build.rs#L76

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link_lib does not handles sub-libraries neither. Is the code before my pull request also broken?
https://github.com/DanielKeep/rust-build-helper/blob/master/src/lib.rs#L584-L589

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before your PR, I think the rust bindings would only use existing lib...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the systems in which cmake or make. does not emit single static library, how did the users provide the static library for rust bindings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, normal compilation steps provide libunicorn.a but cmake doesn't?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me the difference between "normal compilation steps" and cmake called in build script?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me the difference between "normal compilation steps" and cmake called in build script?

There is no difference. It doesn't matter how libunicorn.a is built. You need all static libraries to perform static link, that's all.

Copy link
Author

@Hakuyume Hakuyume Oct 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If "normal compilation steps" can provide libunicorn.a in any systems, using "normal compilation steps" in build script will solve the problem, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If "normal compilation steps" can provide libunicorn.a in any systems, using "normal compilation steps" in build script will solve the problem, I think.

It really doesn't matter how you build libunicorn.a. If you have an inspection of how libunicorn.a contains, it is just a bundle of object files, where you built by whatever building systems. Considering libunicorn.a doesn't contain all objects files and all symbols, you have to provide all static libraries. In fact, that's the work cmake will do in building steps, by resolving dependency and appending all static libs.

At least, a single libunicorn.a won't work on macOS while I also think it won't work on Windows. Only Linux may allow you to specify a single static library and but it depends on how the linker works in this case.

Some reference to make it clear: https://stackoverflow.com/questions/52038439/static-libraries-linked-against-other-static-libraries-with-cmake-one-works-o

@wtdcode
Copy link
Member

wtdcode commented Oct 14, 2021

@Hakuyume If you are still confused, just have a try like this:

/t/t/build $ cat ../CMakeLists.txt
project(test)

add_library(lib1 STATIC lib1.c)

add_library(lib2 STATIC lib2.c)

target_link_libraries(lib2 lib1)

add_executable(a a.c)
target_link_libraries(a lib2)
/t/t/build $ cmake..
# Some log
/t/t/build $ make VERBOSE=1
# Some logs
[100%] Linking C executable a
/usr/local/Cellar/cmake/3.19.1/bin/cmake -E cmake_link_script CMakeFiles/a.dir/link.txt --verbose=1
/Library/Developer/CommandLineTools/usr/bin/cc  -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/a.dir/a.c.o -o a  liblib2.a liblib1.a
# Other logs

As you can see, the executable a is linked against both liblib2.a and liblib1.a and if you tried to build only with liblib2.a, there probably would be some symbols missing.

@Hakuyume
Copy link
Author

Hakuyume commented Oct 14, 2021

I know what your workaround does. It will solve the problem.

I'm confused with #1444 (comment) .
According this comment, the build script before my PR works even in systems in which cmake does not provide a stand-alone libunicorn.a.
I cannot understand how users build a stand-alone libunicorn.a in such systems.

@Hakuyume
Copy link
Author

If #1444 (comment) mentioned only the functionality of the build script and did not consider the preparation of libunicorn.a, everything is clear to me.

@wtdcode
Copy link
Member

wtdcode commented Oct 14, 2021

According this comment, the build script before my PR works even in systems in which cmake does not provide a stand-alone libunicorn.a.

Obviously, no. At least it won't work on Windows, haha.

@Hakuyume
Copy link
Author

OK
I thought your comment pointed some degradation.

@Hakuyume
Copy link
Author

I don't feel hard-coding all possible libraries good.
Can we rely on pkg-config information generated by cmake?
In my system (Linux), it contains enough information for linking.

@wtdcode
Copy link
Member

wtdcode commented Oct 14, 2021

I don't feel hard-coding all possible libraries good. Can we rely on pkg-config information generated by cmake? In my system (Linux), it contains enough information for linking.

That's not available on Windows and you have to handle that.

@Hakuyume
Copy link
Author

Hakuyume commented Oct 15, 2021

That's not available on Windows and you have to handle that.

Since the build script before my PR does not support windows (as you mentioned in #1444 (comment)), Ignoring windows is not degradation.
Developing for windows and macOS is hard for me because I don't have windows and macOS environment.
Can I go without supporting such systems?
Someone who has windows can add windows support on my PR.

@wtdcode
Copy link
Member

wtdcode commented Oct 17, 2021

That's not available on Windows and you have to handle that.

Since the build script before my PR does not support windows (as you mentioned in #1444 (comment)), Ignoring windows is not degradation. Developing for windows and macOS is hard for me because I don't have windows and macOS environment. Can I go without supporting such systems? Someone who has windows can add windows support on my PR.

FIxed in 6d0d089 and CI.

The supported arch list is not too complex to maintain manually and that's easy to do.

@wtdcode wtdcode closed this Oct 17, 2021
@Hakuyume Hakuyume deleted the build-in-build-script branch October 28, 2021 11:10
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.

6 participants