erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
900 stars 194 forks source link

Windows build environment for dynamic linking of ZeroMQ #249

Closed hansieodendaal closed 5 years ago

hansieodendaal commented 5 years ago

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

hansieodendaal commented 5 years ago

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 commented 5 years ago

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

hansieodendaal commented 5 years ago

Hi Andreas,

Thank you for the response:

Your thoughts?

rotty commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Hi Andreas,

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

rotty commented 5 years ago

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