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

Added vendored build support #267

Merged
merged 3 commits into from
May 26, 2019
Merged

Added vendored build support #267

merged 3 commits into from
May 26, 2019

Conversation

jean-airoldie
Copy link
Contributor

@jean-airoldie jean-airoldie commented May 23, 2019

This is the reopening of #262 since github does not allow the reopening of PRs after you force push.

The vendored crate now compiles statically, adding dynamic compilation down the line would be trivial.

@hansieodendaal
Copy link
Contributor

Hi there, I get this error when trying to compile on Windows, using Visual Studio 2017 Developer Command Prompt v15.0:

C:\Code\zmq\rust-zmq-jean-airoldie>cargo build --features vendored
   Compiling zmq-sys v0.9.0 (C:\Code\zmq\rust-zmq-jean-airoldie\zmq-sys)
error: could not find native static library `zmq`, perhaps an -L flag is missing?

error: aborting due to previous error

error: Could not compile `zmq-sys`.

What is the instructions for compiling on Windows?

@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 24, 2019

I have no experience with compiling on windows and I don't have acces to a windows build but i'll try to help.

The error means that this line is not linking to the library properly, which means that lib_dir is the not the right directory. I think this might be related to this line.

After building for cmake, the binaires will be located in a lib directory. However, the name of this directory changes depending on the os.

The easiest way to test would be to clone zeromq-src then run cargo test --all. Then tell me what the directories looks like.

Here is an example from my end:

$ ls target/debug/build/testcrate-5cb3af760001bbb1/out
`build/  include/  lib64/  share/`
$ ls target/debug/build/testcrate-5cb3af760001bbb1/out/lib64/
libzmq.a  pkgconfig/

In my case, the lib directory would be lib64 since it contains libzmq.a, the static library we want to link against.

@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 24, 2019

I'll cook try to cook up a appveyor script to try to build on windows.
edit: This is my WIP PR.

@jean-airoldie
Copy link
Contributor Author

Yep running on appveyor I get the same error:

error: could not find native static library `zmq`, perhaps an -L flag is missing?

@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 24, 2019

Seems like the compiled static lib is called libzmq-v141-mt-sgd-4_3_2.lib but cargo expects zmq.lib. You got any idea why?

@hansieodendaal
Copy link
Contributor

Jean,

Yes, I could also replicate the same behavior on my local system with the appveyer script. The folder structure is shown below, with important files shown. The dll files must be accessible via path. The two libzmq-v141-mt-sgd-4_3_1.lib files listed are exactly the same.

C:\Code\zmq\rust-zmq-jean-airoldie\target\debug\build\zmq-sys-eaf48a98745f7ff2>tree /f
Folder PATH listing for volume Windows
Volume serial number is 04F5-2962
C:.
│   ...
│
└───out
    ├───bin
    │       concrt140.dll
    │       msvcp140.dll
    │       vcruntime140.dll
    │
    ├───build
    │   │   ...
    │   │
    │   ├───bin
    │   │   └───Debug
    │   │       │   ...
    │   │
    │   ├───CMakeFiles
    │   │   │   ...
    │   │
    │   ├───doc
    │   ├───lib
    │   │   └───Debug
    │   │           libzmq-v141-mt-sgd-4_3_1.lib
    │   │           unity.lib
    │   │
    │   ├───libzmq-static.dir
    │   │   └───Debug
    │   │   │   ...
    │   │
    │   ├───tests
    │   │   │   ...
    │   │
    │   ├───unittests
    │   │   │   ...
    │   │
    │   └───x64
    │       └───Debug
    │           │   ...
    │
    ├───CMake
    │       ...
    │
    ├───include
    │       zmq.h
    │       zmq_utils.h
    │
    ├───lib
    │   │   libzmq-v141-mt-sgd-4_3_1.lib
    │   │
    │   └───pkgconfig
    │           ...
    │
    └───share
        └───zmq
                ...

Running cargo test --all produced this output:

error: failed to run custom build command for `zmq-sys v0.9.0 (C:\Code\zmq\rust-zmq-jean-airoldie\zmq-sys)`
process didn't exit successfully: `C:\Code\zmq\rust-zmq-jean-airoldie\target\debug\build\zmq-sys-56d4f05976e51776\build-script-main` (exit code: 101)
--- stderr
thread 'main' panicked at 'Unable to locate libzmq:
Failed to run `"pkg-config" "--libs" "--cflags" "libzmq >= 4.1"`: The system cannot find the file specified. (os error 2)', zmq-sys\build\pkg_config.rs:25:17
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

warning: build failed, waiting for other jobs to finish...
error: build failed

@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 24, 2019

I meant running cargo test --all -vv from zeromq-src.

I'm not sure I understand what you meant by accessible via PATH. I assume that outputting this to cargo should be enought. On unix this would add the directory to the rustc search path.

edit: Added -vv flag so we see the build script in action.

@hansieodendaal
Copy link
Contributor

Yes, I did that from your branch. The lib files reference the dll files in the dynamic case, the latter to be accessible via path. See comments in your WIP PR.

@oblique
Copy link
Contributor

oblique commented May 25, 2019

Tested cross-compilation for ARM Linux and works.

@jean-airoldie
Copy link
Contributor Author

I'll bump the version.

@jean-airoldie jean-airoldie changed the title Added vendored build support WIP Added vendored build support May 25, 2019
@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 25, 2019

I'll add CI testing for the feature for both release and debug since it changes how libzmq is compiled.

@jean-airoldie jean-airoldie force-pushed the zeromq-src branch 4 times, most recently from 9737c2e to 9270f3e Compare May 25, 2019 16:02
@jean-airoldie jean-airoldie force-pushed the zeromq-src branch 8 times, most recently from 19eed59 to 6f42a4e Compare May 25, 2019 21:16
@jean-airoldie jean-airoldie changed the title WIP Added vendored build support Added vendored build support May 25, 2019
@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 25, 2019

Note that until #268 is merged, the CI will fail.

edit: Or not...

@jean-airoldie jean-airoldie force-pushed the zeromq-src branch 2 times, most recently from 81764ba to 053acc3 Compare May 25, 2019 21:22
@hansieodendaal
Copy link
Contributor

hansieodendaal commented May 26, 2019

I ran these tests in Windows on this latest branch in my own environment (not Appveyer style) using pre-built release version libzmq libraries for the dynamic linking:

This pass:

cargo clean
cargo test
cargo test --release

This pass:

cargo clean
cargo test --all-features
cargo test --release --all-features

This fails (do not know if the sequence is valid though):

cargo clean
cargo build --features vendored
cargo test
cargo test --release

build test 1.txt

Rust environment:

C:\Users\pluto\Documents\Code\zmq\rust-zmq-jean-airoldie>rustc --version
rustc 1.36.0-nightly (af98304b9 2019-05-11)

C:\Users\pluto\Documents\Code\zmq\rust-zmq-jean-airoldie>cargo --version
cargo 1.36.0-nightly (759b6161a 2019-05-06)

These instructions apply for the dynamic linking case as per PR #249:

Note for Windows users (re. dynamic linking of ZeroMQ):

  • 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.
  • The folder containing the *.dll (dynamic link library)
    referred to by zmq.lib must be accessible via the path for
    the session that invokes the Rust compiler.
  • The name of the *.dll in question depends on the build system
    used for libzmq and can usually be seen when opening zmq.lib
    in a text editor.

@jean-airoldie
Copy link
Contributor Author

From my understanding the failing tests are due to #268.

@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 26, 2019

Specifically:

cargo clean
cargo test --release

should fail even on the master branch currently because some tests assume that target/debug exists.

@hansieodendaal
Copy link
Contributor

hansieodendaal commented May 26, 2019

After applying patch code from PR #268 to this branch these also work:

cargo clean
cargo test --release

cargo clean
cargo test --release --all-features

cargo clean
cargo test --release --features vendored

build test 2.txt

@hansieodendaal
Copy link
Contributor

hansieodendaal commented May 26, 2019

I also ran these tests in Windows on this latest branch Appveyer style on my own system, but with patches applied from PR #268, with an updated appveyer.bat to enable running it locally (appveyor_local.bat), including the test sequences below. All tests passed.

cargo clean
cargo test

cargo clean
cargo test --release

cargo clean
cargo test --all-features 

cargo clean
cargo test --release --all-features 

cargo clean
cargo test --features vendored

cargo clean
cargo test --release --features vendored

build test 3.txt

Copy link
Collaborator

@rotty rotty left a comment

Choose a reason for hiding this comment

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

Besides the one, not substantial, Windows issue, this looks really good. I look forward to release 0.9.1 with this added feature in the release notes! @jean-airoldie, if you feel inclined, I'd be happy for you to add a NEWS entry regarding this feature, otherwise I'll cook something up before releasing 0.9.1.

cargo test --all-features %CARGO_MODE%
if %ERRORLEVEL% NEQ 0 exit 1

cargo test --release --all-features %CARGO_MODE%
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line doesn't match up with how the Windows build is structured, as %CARGO_MODE% is either empty or already set to --release. I guess the correct thing to do instead is to comment in the Release configuration line in .appveyor.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll uncomment the Release in appveyor.yml, if thats what you meant.

* Enables building statically from source by specifying the `vendored` flag.
* Adds an optional `zeromq-src` dependency.
@jean-airoldie
Copy link
Contributor Author

I'll add a NEWS entry.

@rotty rotty merged commit 3044c8d into erickt:master May 26, 2019
@rotty
Copy link
Collaborator

rotty commented May 26, 2019

Thanks a lot!

@rotty rotty mentioned this pull request May 27, 2019
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.

4 participants