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

add windows msvc support #31

Merged
merged 2 commits into from
Dec 16, 2018
Merged

Conversation

xmclark
Copy link
Contributor

@xmclark xmclark commented Dec 14, 2018

Build on windows, #20. Today, wabt-rs does not compile on windows with msvc. Pretty much all of the pieces are in place. The only thing missing is a little extra glue where we call CMake.

This PR takes inspiration from this comment.

This PR adds some conditional compilation steps when running CMake. Building windows will cause the profile to be set based on the PROFILE environment variable. The rust profile names match the msvc profile names. Thanks to suggestion from @sanxiyn, instead we glob for the wabt.lib and let cmake handle the profile as it does by default.

I don't know of a more elegant solution to conditionally compiling a single builder method call, so I had to duplicate the code. I am open to new ideas.

The call to cargo:rustc-link-search=native= is added to link the library in the correct target folder e.g. target/debug.
The solution has been updated. A new build dependency is added: glob. The solution is pretty simple to understand.

I built on my windows 10 computer. I also added an appveyor config for automated CI builds. It currently builds rust stable with visual studio 2017 only.


My implementation seems complicated. I feel like there is a possibly a simpler way to do this. Explicitly setting the profile just for windows adds a lot of extra code. I am looking for feedback!

Open question:
Is there a negative impact to linux builds if the CMake profile is always explicitly set? In other words, do we need to conditional compilation?

Answer: we are going to let cmake handle the profile instead. See new solution above.

@xmclark xmclark changed the title WIP - include target profile libs in build add windows msvc support Dec 14, 2018
@sanxiyn
Copy link

sanxiyn commented Dec 14, 2018

I think we can just ignore the profile and look for the library in subdirectories.

let mut out_lib_dir = out_build_dir.clone();
if cfg!(target_os = "windows") {
    let pattern = format!("{}/*/wabt.lib", out_build_dir.display());
    for entry in glob::glob(&pattern).unwrap() {
        if let Ok(path) = entry {
            out_lib_dir = path.parent().unwrap().to_path_buf();
            break;
        }
    }
}

@xmclark
Copy link
Contributor Author

xmclark commented Dec 14, 2018

@sanxiyn thanks for feedback!

Does the profile really not matter? I assumed that it would matter because the ABI might be different between release and debug.

Do you prefer using a glob instead of an explicit path because it will avoid any differences between windows target directories?

I think windows rust will always use the same target folder structure, but I'm not totally sure. This solution would find it regardless. I think the risk of multiple different wabt.lib being generated is pretty low.

@pepyakin
Copy link
Owner

fwiw, I just enabled appveyor but haven’t had a chance to test it.

@sanxiyn
Copy link

sanxiyn commented Dec 16, 2018

The idea is to let cmake crate choose the profile and use whatever is chosen. There won't be multiple wabt.lib since only one profile is built.

xmclark's approach is to compute CMake profile from Cargo profile. It duplicates the same logic in cmake crate (see here) incompletely, for example CMake profile RelWithDebInfo won't be chosen.

@xmclark
Copy link
Contributor Author

xmclark commented Dec 16, 2018

@sanxiyn that is a good point. I see in the docs for cmake that it will infer the profile. I will implement your suggested solution when I get home! Thanks!

@xmclark xmclark force-pushed the windows-support branch 2 times, most recently from 0c01c99 to c4040ee Compare December 16, 2018 16:59
@xmclark
Copy link
Contributor Author

xmclark commented Dec 16, 2018

I've updated the PR description. I think this is ready for review.

Copy link
Owner

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

Great! Thank you!

let mut out_build_dir = dst;
out_build_dir.push("build");

println!("cargo:rustc-link-search=native={}", out_build_dir.display());

// help cargo find wabt.lib when targeting windows
#[cfg(windows)] {
Copy link
Owner

Choose a reason for hiding this comment

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

oh, wow, didn't know this is a thing!

@pepyakin pepyakin merged commit 4a5d05b into pepyakin:master Dec 16, 2018
@pepyakin
Copy link
Owner

(CI checks pass, I just added a new line to Cargo.toml and skiped checks)

@xmclark xmclark deleted the windows-support branch December 16, 2018 19:06
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.

3 participants