Windows build environment for dynamic linking of ZeroMQ #249

hansieodendaal · 2019-03-25T10:52:01Z

This change to assist Widows users to build rust-zmq when using ZeroMQ dynamic link libraries. Changes incorporated:

some Windows specific build instructions;
adding a function to zmq-sys/build.rs to verify the Windows build environment and
adding 2x helper batch files to assist users in setting up their Windows environment after compilation of ZeroMQ.

hansieodendaal · 2019-03-26T11:48:40Z

This cargo test --verbose --features unstable-testing seems broken for rustc 1.35.0-nightly (4c27fb19b 2019-03-25) but succeeds for rustc 1.35.0-nightly (e68bf8ae1 2019-03-11)

failing-rustc-1.35.0-nightly-2019-03-25-Ubuntu.txt
failing-rustc-1.35.0-nightly-2019-03-25-Windows10.txt
success-rustc-1.35.0-nightly-2019-03-25-Windows10.txt

rotty · 2019-05-19T22:46:44Z

Sorry, I haven't had time to give your build system changes a proper review, but here are some initial thoughts:

The Windows build instructions are certainly very welcome, especially as I don't privately run Windows, so I have no idea of any peculiarities there.
There is a somewhat related issue, zeromq-src #257, and PR Introduce static-libzmq crate feature for easier building of zmq crate. #241. I hope that eventually we will end up with a vendored feature flag that makes static linking against libzmq much easier, using the proposed libzmq-src crate. I'm not yet sure if this approach is applicable for dynamic linking, though, which is important due to the LGPL license of libzmq.
The .bat scripts you supplied are, to be honest, hard to look at, to the point where I don't want to review them. They seem overly long for what they are supposed to accomplish (if I understand their purpose correctly). They also seem extremely repetitive. I'm sure this is due to cmd just being a horrible language. Since they seem to require powershell anyway, perhaps doing this in powershell would improve the legibility to the point where they become reviewable to me?

hansieodendaal · 2019-05-20T08:50:40Z

Hi Andreas,

Thank you for the response:

I have looked at the PRs you mentioned, the one I am very familiar with.
You are right, coding in cmd is not pleasant, and not easy to read. Making them robust seems to make them repetitive. The batch files could be completely removed if we put some more instructions in the README.md file if you prefer that, however, this would defeat the purpose.
The only way to really review the Windows scripts are to run them in a Windows environment. These batch files are only helper functions to prepare a user's environment and cannot break rust-zmq.
Very few Windows users are familiar with PowerShell, including me. I only use it when I really have to, for example to reliably clear registry entries from within the cmd environment or to change the case of command line parameters.
The length of the path environment variable in Windows is limited, and depending on what a user have installed it is not practical for everyone, including me, to permanently add LIBZMQ_BIN_DIR to their path, but rather to source it when needed.
PowerShell and cmd do not play well together when setting session paths. Each can inherit the environment from the other, but one cannot call a PowerShell script from cmd to change the session path for the cmd environment and vice versa.
The cmd file win_set_environment_vars.bat works equally well when called from either cmd or PowerShell, so I propose we leave that one as is. It is also only run once to add the LIBZMQ* environment variables persistently to the registry.
I can add a win_source_dll_path.ps1 script for users who want to work straight from a PowerShell environment without instantiating it from the cmd environment.

Your thoughts?

rotty · 2019-05-20T18:45:43Z

You are right, coding in cmd is not pleasant, and not easy to read. Making them robust seems to make them repetitive. The batch files could be completely removed if we put some more instructions in the README.md file if you prefer that, however, this would defeat the purpose.

Having more extensive instructions would be great! I'd feel more comfortable with that than with scripts I cannot be bothered to understand ;-). Maybe it makes sense to wait on how PR #262 works out, befor adding more extensive Windows build instructions, as that PR will have implications on the build instructions for all platforms.

The only way to really review the Windows scripts are to run them in a Windows environment. These batch files are only helper functions to prepare a user's environment and cannot break rust-zmq.

I'm aware of that, but the thing I'm not comfortable with is that users in the future might be running into issues with these scripts, reporting them on github, and no-one can help them out.

hansieodendaal · 2019-05-21T11:39:07Z

Hi Andreas,

I updated the Windows build instructions for dynamic linking and removed the batch files from the system (but left a GitHub gist link in case anyone want to use them).

I checked out PR #262; my proposed changes can easily be integrated and is not conflicting in any way.

Regards

rotty

IIUC, what verify_windows_build_environment is verifying that the user has set up their PATH correctly, so that the libzmq DLL can be found at runtime (e.g. when running tests). I think this is not the build scripts' business to check. The code you wrote will also be invoked when cross-compiling from Linux to the x86_64-pc-windows-gnu target, which works fine on current master and release/v0.8 branches, but will fail with your changes. So, I think checking this in build.rs is not reasonable.

Furthermore, the code seems to only add a level of indirection -- it doesn't actually verify that zmq.dll can be found in the PATH environment variable, but only that another environment variable, namely ZMQ_BIN_DIR is set and found in PATH. If you get setting that environment variable wrong, you will end up with same error message, presumably that zmq.dll could not be found, when trying to run (note: not build) zmq.

So overall, I'd propose removing any references to LIBZMQ_BIN_DIR from the instructions, and mention what really counts: that the shared library produced by the MSVC build must be named zmq.dll and be placed in a directory that is found in PATH. Then, the change to the appveyor batch file is superfluous. If I'm not wrong in my analysis, and this is indeed the one thing Windows developers have to ensure, the links to your complex .bat scripts also seem to have little value.

So unless I've misunderstood the situation, please drop all code changes and the .bat link and let's make this PR just about Windows build instructions.

rotty · 2019-05-22T20:53:50Z

zmq-sys/build.rs

@@ -3,6 +3,27 @@ extern crate metadeps;
 use std::env;
 use std::path::Path;

+fn verify_windows_build_environment() {
+    let target = env::var("TARGET").ok().unwrap();
+    println!("cargo:target={:?}", target);


Why is this cargo:target config necessary? According to the cargo book, TARGET should already refer to the target triplet selected, so this looks like a no-op to me. Also, I could not find any mention of the cargo:target key in the build script section of the book.

I cannot answer you really, just wanted to determine if the target is Windows, and this got the job done.
I just used the reference to cargo:target to print some meaningful information for the user in the event that the code panics.
Will remove fn verify_windows_build_environment() altogether as suggested to default back to the original, so this will be resolved.

rotty · 2019-05-22T21:07:28Z

README.md

+
+- When building `libzmq` from sources, the library must be renamed 
+  to `zmq.lib` from the auto named `libzmq-v***-mt-gd-*_*_*.lib`, 
+  `libzmq.lib`, `libzmq-mt-*_*_*.lib`, etc. 


Stylistic note: I assume you intend to have a paragraph break after etc.. If so, you need a separate blank line. If not, you should your IDE/editor's facilities to block-justify the whole (then single) paragraph. Right now, it looks like a missing blank line (i.e., paragraph break). Similiar instances below. .

Markdown will render this as close spaced bullets. With empty lines between the bullets the spacing increases. See GitHub screenshot below:

Oh, sorry, I missed this dashes next to the plus signs of the diff.

rotty · 2019-05-22T21:10:41Z

zmq-sys/build.rs

+        let dll_path = prefix_dir("LIBZMQ_BIN_DIR", "bin");
+        match &dll_path {
+            Some(dll_path) => {
+                println!("cargo:dll_path={}", &dll_path);


Likewise, I couldn't dig up any documentation about cargo:dll_path.

I just used the reference to cargo:dll_path to print some meaningful information for the user in the event that the code panics.
Will remove fn verify_windows_build_environment() altogether as suggested, so this will be resolved.

rotty · 2019-05-22T21:49:17Z

Oh, and additionally, please squash (and if, applicable, rebase) your changes, giving the resulting squashed commit a fitting commit message. I prefer to have a simple git history, instead of preserving the the evolution of each PR for posterity in the git repository, as you would get when adding fixup commits to a PR. A PR should only contain multiple commits if they are separate logical units, which can be understood on their own.

I really need to write up that CONTRIBUTIONS.md document.

hansieodendaal · 2019-05-23T13:12:18Z

Hi Andreas,

All suggestions implemented. Just need to do the squash & rebase bit. ... Done!

…-zmq into release/v0.8 Updated Windows build instructions for dynamic linking of ZeroMQ

rotty · 2019-05-26T19:35:26Z

I've now squash-merged your release/v0.8 branch, modulo some whitespace adjustments, and will merge release/v0.8 into master soonish. Thanks!

Windows build environment for dynamic linking of ZeroMQ

da6de13

This was referenced Mar 25, 2019

Build instructions for Windows? #208

Closed

Problem with Windows build #213

Closed

Add LIBZMQ_BIN_DIR environment variable for AppVeyor automatic build

ad9f67e

hansieodendaal added 2 commits May 21, 2019 07:19

Removed batch files from system, updated Windows build instructions.

d710583

Updated instructions, fixed folder bug

c4a3907

hansieodendaal mentioned this pull request May 21, 2019

windows_dynamic_link_support jean-airoldie/rust-zmq#1

Closed

rotty requested changes May 22, 2019

View reviewed changes

Removed all code changes, updated Windows build instructions

30bac31

hansieodendaal added 2 commits May 24, 2019 01:11

Windows build environment for dynamic linking of ZeroMQ

aec38bd

Merge branch 'release/v0.8' of https://github.com/hansieodendaal/rust…

868597d

…-zmq into release/v0.8 Updated Windows build instructions for dynamic linking of ZeroMQ

hansieodendaal mentioned this pull request May 26, 2019

Added vendored build support #267

Merged

rotty closed this May 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows build environment for dynamic linking of ZeroMQ #249

Windows build environment for dynamic linking of ZeroMQ #249

hansieodendaal commented Mar 25, 2019

hansieodendaal commented Mar 26, 2019

rotty commented May 19, 2019

hansieodendaal commented May 20, 2019

rotty commented May 20, 2019 •

edited

Loading

hansieodendaal commented May 21, 2019

rotty left a comment

rotty May 22, 2019

hansieodendaal May 23, 2019

rotty May 22, 2019

hansieodendaal May 23, 2019

rotty May 26, 2019

rotty May 22, 2019

hansieodendaal May 23, 2019

rotty commented May 22, 2019

hansieodendaal commented May 23, 2019 •

edited

Loading

rotty commented May 26, 2019

Windows build environment for dynamic linking of ZeroMQ #249

Windows build environment for dynamic linking of ZeroMQ #249

Conversation

hansieodendaal commented Mar 25, 2019

hansieodendaal commented Mar 26, 2019

rotty commented May 19, 2019

hansieodendaal commented May 20, 2019

rotty commented May 20, 2019 • edited Loading

hansieodendaal commented May 21, 2019

rotty left a comment

Choose a reason for hiding this comment

rotty May 22, 2019

Choose a reason for hiding this comment

hansieodendaal May 23, 2019

Choose a reason for hiding this comment

rotty May 22, 2019

Choose a reason for hiding this comment

hansieodendaal May 23, 2019

Choose a reason for hiding this comment

rotty May 26, 2019

Choose a reason for hiding this comment

rotty May 22, 2019

Choose a reason for hiding this comment

hansieodendaal May 23, 2019

Choose a reason for hiding this comment

rotty commented May 22, 2019

hansieodendaal commented May 23, 2019 • edited Loading

rotty commented May 26, 2019

rotty commented May 20, 2019 •

edited

Loading

hansieodendaal commented May 23, 2019 •

edited

Loading