Skip to content
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

fix rust bindings build on windows #1584

Merged
merged 11 commits into from
Apr 16, 2022
Merged

Conversation

shuffle2
Copy link
Contributor

@shuffle2 shuffle2 commented Apr 8, 2022

Some things I noticed while trying to get unicorn-engine to work on windows/rust:

First I just tried

unicorn-engine = "2.0.0-rc6"

and I got

--- stderr
  thread 'main' panicked at 'Fail to create build directory on Windows.: Error { kind: NotFound, message: "program not found" }'

This (IMO) is kind of nonsense error. because the problem was really that it could not find cmake.

OK, so I run "c:\Program Files\Microsoft Visual Studio\2022\Community\vc\Auxiliary\Build\vcvarsall.bat" x64 and try again:

  = note: LINK : fatal error LNK1181: cannot open input file 'unicorn.lib'

ok...so I head to this github and see that there are related changes in dev branch, so I do this:

unicorn-engine = { git = "https://github.com/unicorn-engine/unicorn.git", branch = "dev" }

but still get

  = note: LINK : fatal error LNK1181: cannot open input file 'unicorn-static.lib'

and, interestingly, checking in target\release\build\unicorn-engine-2c01a69f559d00b5\output, I see:

cargo:warning=Unicorn not found. Downloading...
cargo:rustc-link-search=<mypath>\target\release\build\unicorn-engine-2c01a69f559d00b5\out\unicorn-engine-unicorn-c10639f\build_rust\Release

This means that build.rs of unicorn has downloaded tarball of master branch even though it's executing from dev branch checkout (note c10639f in the path). fwiw this is problematic even when checking out dev branch of unicorn manually and using unicorn-engine = { path = "unicorn/bindings/rust" }

So...:

  1. I'm not that familiar with crates that need to invoke cmake from build.rs, but surely there is some better way than requiring the user to start cargo from a msvc build environment? Or would this not be a problem if the project had a real, working crate published? idk
  2. IMO the cmake build generator should be Ninja, not VS. this can be relied upon to exist in VS env, but idk if you want to rely on it for other platforms
  3. the download step of build.rs should be aware of cargo settings such as the wanted branch (I have no idea how to do that, or if it's the proper thing, just pointing it out :D) I eventually had to set UNICORN_LOCAL, clean build tree, and retry to get it working, which IMO is kinda gross.

This PR at least makes it build, but doesn't really fix the above problems.
What it does:

  1. use ninja cmake generator on all platforms
  2. use cmake --build . --parallel to have cmake automagically do the build, which should work everywhere. (this is handled by cmake crate)
  3. uses build dir for rustc-link-search
  4. removes the "download unicorn" logic, which was broken and not useful.

@wtdcode
Copy link
Member

wtdcode commented Apr 12, 2022

Hello, the Windows rust build is indeed broken these days and I'm trying to fix it.

IMO the cmake build generator should be Ninja, not VS. this can be relied upon to exist in VS env, but idk if you want to rely on it for other platforms

Yes, ninja is better in this case but still I don't think you can really build out of VS prompt.

the download step of build.rs should be aware of cargo settings such as the wanted branch (I have no idea how to do that, or if it's the proper thing, just pointing it out :D) I eventually had to set UNICORN_LOCAL, clean build tree, and retry to get it working, which IMO is kinda gross.

Exactly. It's controlled by an environment variable for now and not ideal I admit.

bindings/rust/build.rs Outdated Show resolved Hide resolved
bindings/rust/build.rs Outdated Show resolved Hide resolved
@wtdcode
Copy link
Member

wtdcode commented Apr 12, 2022

Another consideration of not using ninja is that ninja is not shipped with MSVC while msbuild absolutely will.

@shuffle2
Copy link
Contributor Author

Another consideration of not using ninja is that ninja is not shipped with MSVC while msbuild absolutely will.

ninja will be present if cmake is, so it's not a problem for msvc/VS. To be clear, with my changes i can build my project (which depends on unicorn-engine) cleanly, and not have to start VS environment manually.

@shuffle2
Copy link
Contributor Author

It looks like current version of this PR will work fine if I just make it fallback to cmake's default generator if ninja isn't found. That would solve all the problems i've listed, while still keeping the matrix of CI builders happy. IDK about things that are not in CI (like mingw on msys2).

@shuffle2 shuffle2 force-pushed the win-rust branch 3 times, most recently from ee11802 to d0cad65 Compare April 13, 2022 10:15
@shuffle2
Copy link
Contributor Author

sorry, i had a goof, it should actually work now..i think

@shuffle2
Copy link
Contributor Author

it should be ready to go now

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@wtdcode
Copy link
Member

wtdcode commented Apr 15, 2022

Looks good to me! I really appreciate your work and it could be merged once I test it locally.

Thanks!

@shuffle2
Copy link
Contributor Author

Btw, I notice that the base windows environments provided for GitHub actions already include all the tools you use (msbuild,cmake,ninja,etc) : windows server 2022. Additionally it includes older versions of the tool chains (you can target vs2019, vs2022, etc ) from same image. So, your ci flow can be sped up a little by skipping some steps which are redundant (not for this pr, tho)

@wtdcode wtdcode merged commit 2912cd1 into unicorn-engine:dev Apr 16, 2022
@wtdcode
Copy link
Member

wtdcode commented Apr 16, 2022

Btw, I notice that the base windows environments provided for GitHub actions already include all the tools you use (msbuild,cmake,ninja,etc) : windows server 2022. Additionally it includes older versions of the tool chains (you can target vs2019, vs2022, etc ) from same image. So, your ci flow can be sped up a little by skipping some steps which are redundant (not for this pr, tho)

Yes, the CI setup is quite old and can be optimized in many ways but that's not prioritized too much for now.

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