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 support of vcpkg with msvc toolchain #79

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lapoigne
Copy link

@lapoigne lapoigne commented Mar 3, 2021

I added the support of vcpkg libraries in proj-sys.
This allow to build proj-sys on windows with msvc without pkg-config.

To use vcpkg libraries on your rust program, you need:

  • in .cargo/config file
[target.x86_64-pc-windows-msvc]
linker = "rust-lld.exe"
rustflags = ["-Ctarget-feature=+crt-static"]
  • in Cargo.toml file
[package.metadata.vcpkg]
git = "https://github.com/microsoft/vcpkg"
rev = "96a1f9c" # PROJ version 7.2.1 - https://github.com/microsoft/vcpkg/search?q=proj4&type=commits
install = ["proj:x64-windows-static"]

To build, run

cargo install cargo-vcpkg
cargo vcpkg build
cargo build

I'm new to rust, I hope this isn't to ugly in my implementation.

@michaelkirk
Copy link
Member

AFAIK none of the main contributors are using windows, so it'd be good to test this setup in CI.

Do you have any experience with using windows containers in GH actions? Would you be willing to add a job to https://github.com/georust/proj/blob/master/.github/workflows/test.yml?

@urschrei
Copy link
Member

urschrei commented Mar 9, 2021

Hi @lapoigne, apologies for the delay. As @michaelkirk says, we don't have anyone available to locally test this, but we're certainly interested in anything that improves the Windows experience for these crates.

@lapoigne
Copy link
Author

lapoigne commented Mar 9, 2021

I will look closer to add CI for windows platform.

@lapoigne
Copy link
Author

I added windows in CI file.
The 32bit platform is commented because we loose precision in floating number and the test fail.
Maybe you just want remove this architecture.
I edited some test because I get some insignificant change in some value like 1450880.2910605008 for 1450880.2910605003

@michaelkirk
Copy link
Member

bors try

Sorry to let this sit for so long. Thanks for adding the ci integration, let's try it out.

@michaelkirk
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Apr 19, 2021

try

Already running a review

@michaelkirk
Copy link
Member

bors r-

@michaelkirk
Copy link
Member

bors retry

@michaelkirk
Copy link
Member

bors r+

one last try bors.

@michaelkirk
Copy link
Member

michaelkirk commented Apr 19, 2021

Bors is crashing, I'm going to follow up with some bors peeps.

update: done here https://forum.bors.tech/t/bors-crashing-resource-not-accessible-by-integration/571/3

update 2: looking into similarly errors reported, it seems like "editing the workflow file" is a common source of crashes between bors and gh

@michaelkirk michaelkirk mentioned this pull request Apr 19, 2021
bors bot added a commit that referenced this pull request Apr 19, 2021
@bors
Copy link
Contributor

bors bot commented Apr 19, 2021

try

Build succeeded:

proj-sys/build.rs Outdated Show resolved Hide resolved
proj-sys/build.rs Outdated Show resolved Hide resolved
proj-sys/build.rs Outdated Show resolved Hide resolved
proj-sys/build.rs Outdated Show resolved Hide resolved
build::link("ole32", true);
let lib = vcpkg::Config::new()
.emit_includes(true)
.find_package("proj");
Copy link
Member

Choose a reason for hiding this comment

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

We need to constrain the version based on MINIMUM_PROJ_VERSION like we do with pkg_config. Is that possible with vcpkg?

Copy link
Author

Choose a reason for hiding this comment

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

I look you to check the version

Copy link
Author

Choose a reason for hiding this comment

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

I can't get the library version with vcpkg.
I can walk to library parents directory to read json file of the package if really needed

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the incredibly long delay (almost a year!).

The nightmare scenario is that someone has an old unsupported version of libproj and this library then gives them weird results because of it. On non-windows platforms we can enforce that this doesn't happen.

I'm not sure how likely this is in practice. Anybody else want to weigh in?

@wlinna
Copy link

wlinna commented Nov 9, 2022

Using opus-rs as a starting point, I was able to compile proj
rust-av/opus-rs@9f04ec1

Given these assumptions.. :

  • You use 64-bit Windows (I tested on Windows 10)
  • You have chocolatey installed (a package manager for Windows)
  • You have vcpkg installed at C:\src\vcpkg
  • You have an environment variable PKG_CONFIG_PATH set to C:\src\vcpkg\installed\x64-windows\lib\pkgconfig
  • You have LLVM installed and its path is added to Path environment variable (needed for bindgen I guess?)

These commands should work fine. At least I was able to build my project on Windows :

  1. chocolatey install pkgconfiglite (requires elevated privileges)
  2. Open PowerShell in C:\src\vcpkg
  3. .\vcpkg.exe install proj:x64-windows
  4. cargo init
  5. cargo add proj
  6. cargo build

(NOTE: I don't know whether or not this alone will result in linking with a release build of proj C++-library.)

I'm just thinking that maybe instead of referring to vcpkg directly in build.rs and Cargo configurations, georust/proj could consider requiring pkgconfiglite on Windows just like it requires pkg-config on other platforms (?). Of course the process would have to be documented.

I guess one advantage of this approach would be that it might not actually depend on vcpkg since developers can choose their own method for compiling proj C++-library (or so I believe).

I hope this helps

@michaelkirk
Copy link
Member

Hi @wlinna! To clarify — it sounds like you were able to build with the unmodified proj release using the instructions you provided.

Is that right? Or did you have to modify the proj/proj-sys crate in some way?

@wlinna
Copy link

wlinna commented Nov 10, 2022 via email

@kylebarron
Copy link
Member

This is amazing! Thanks for the hint about pkgconfiglite @wlinna. I was able to get proj building on CI for Python in geopolars/geopolars#174

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.

5 participants