search

simonrw / rust-fitsio

FFI wrapper around cfitsio in Rust
Apache License 2.0
36 stars 12 forks source link

Support usage under MS Windows 64-bit #118

Open GreatAttractor opened 4 years ago

GreatAttractor commented 4 years ago

Hello,

Many thanks for providing this crate! Works fine under Linux (Fedora x86-64), but it fails when trying to use as a dependency (v. 0.15.0) under MSW 64-bit (MSYS2/MinGW, rustc 1.42.0, cfitsio 3.450-1):

   Compiling fitsio v0.15.0
error[E0308]: mismatched types
  --> C:\Users\...\.cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\headers.rs:45:25
   |
45 |                         &mut value,
   |                         ^^^^^^^^^^ expected `i32`, found `i64`
...
59 | reads_key_impl!(i64, fits_read_key_lng);
   | ---------------------------------------- in this macro invocation
   |
   = note:    expected raw pointer `*mut i32`
           found mutable reference `&mut i64`

error[E0308]: mismatched types
   --> C:\Users\...\.cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:86:37
    |
86  |                             0 => Ok(out),
    |                                     ^^^ expected `i64`, found `i32`
...
155 | reads_col_impl!(i64, fits_read_col_lng, 0);
    | ------------------------------------------- in this macro invocation
    |
    = note: expected struct `std::vec::Vec<i64>`
               found struct `std::vec::Vec<i32>`

error[E0308]: mismatched types
   --> C:\Users\...\.cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:86:37
    |
86  |                             0 => Ok(out),
    |                                     ^^^ expected `u64`, found `u32`
...
159 | reads_col_impl!(u64, fits_read_col_ulng, 0);
    | -------------------------------------------- in this macro invocation
    |
    = note: expected struct `std::vec::Vec<u64>`
               found struct `std::vec::Vec<u32>`

error[E0308]: mismatched types
   --> C:\Users\...\.cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:140:25
    |
105 |             fn read_cell_value<T>(fits_file: &mut FitsFile, name: T, idx: usize) -> Result<Self>
    |                                                                                     ------------ expected `std::result::Result<i64, errors::Error>` because of return type
...
140 |                         check_status(status).map(|_| out)
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `i64`, found `i32`
...
155 | reads_col_impl!(i64, fits_read_col_lng, 0);
    | ------------------------------------------- in this macro invocation
    |
    = note: expected enum `std::result::Result<i64, _>`
               found enum `std::result::Result<i32, _>`

error[E0308]: mismatched types
   --> C:\Users\...\.cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:140:25
    |
105 |             fn read_cell_value<T>(fits_file: &mut FitsFile, name: T, idx: usize) -> Result<Self>
    |                                                                                     ------------ expected `std::result::Result<u64, errors::Error>` because of return type
...
140 |                         check_status(status).map(|_| out)
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found `u32`
...
159 | reads_col_impl!(u64, fits_read_col_ulng, 0);
    | -------------------------------------------- in this macro invocation
    |
    = note: expected enum `std::result::Result<u64, _>`
               found enum `std::result::Result<u32, _>`

error: aborting due to 5 previous errors

It seems to be an issue with long being 32-bit under MSW 64-bit.

simonrw commented 4 years ago

Hi thanks for this report, I'll be the first to admit the crate is not well tested on windows.

I think you're right with your suggestion. I'll take a look but if you're up for it feel free to give a PR a go.

Thanks

On Fri, 10 Apr 2020, 17:05 Filip Szczerek, notifications@github.com wrote:

Hello,

Many thanks for providing this crate! Works fine under Linux (Fedora x86-64), but it fails when trying to use as a dependency (v. 0.15.0) under MSW 64-bit (MSYS2/MinGW, rustc 1.42.0, cfitsio 3.450-1):

Compiling fitsio v0.15.0 error[E0308]: mismatched types --> C:\Users....cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\headers.rs:45:25 45 &mut value, ^^^^^^^^^^ expected i32, found i64 ... 59 reads_key_impl!(i64, fits_read_key_lng); ---------------------------------------- in this macro invocation

= note: expected raw pointer *mut i32 found mutable reference &mut i64

error[E0308]: mismatched types --> C:\Users....cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:86:37 86 0 => Ok(out), ^^^ expected i64, found i32 ... 155 reads_col_impl!(i64, fits_read_col_lng, 0); ------------------------------------------- in this macro invocation 
= note: expected struct `std::vec::Vec<i64>`
           found struct `std::vec::Vec<i32>`
error[E0308]: mismatched types --> C:\Users....cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:86:37 86 0 => Ok(out), ^^^ expected u64, found u32 ... 159 reads_col_impl!(u64, fits_read_col_ulng, 0); -------------------------------------------- in this macro invocation 
= note: expected struct `std::vec::Vec<u64>`
           found struct `std::vec::Vec<u32>`
error[E0308]: mismatched types --> C:\Users....cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:140:25 105 fn read_cell_value(fits_file: &mut FitsFile, name: T, idx: usize) -> Result ------------ expected std::result::Result<i64, errors::Error> because of return type ... 140 check_status(status).map( _ out) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected i64, found i32 ... 155 reads_col_impl!(i64, fits_read_col_lng, 0); ------------------------------------------- in this macro invocation 
= note: expected enum `std::result::Result<i64, _>`
           found enum `std::result::Result<i32, _>`
error[E0308]: mismatched types --> C:\Users....cargo\registry\src\github.com-1ecc6299db9ec823\fitsio-0.15.0\src\tables.rs:140:25 105 fn read_cell_value(fits_file: &mut FitsFile, name: T, idx: usize) -> Result ------------ expected std::result::Result<u64, errors::Error> because of return type ... 140 check_status(status).map( _ out) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u64, found u32 ... 159 reads_col_impl!(u64, fits_read_col_ulng, 0); -------------------------------------------- in this macro invocation 
= note: expected enum `std::result::Result<u64, _>`
           found enum `std::result::Result<u32, _>`

error: aborting due to 5 previous errors

It seems to be an issue with long being 32-bit under MSW 64-bit.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mindriot101/rust-fitsio/issues/118, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAOS3DMIMKUZJ2QZUROFTTRL47VVANCNFSM4MFRIU3A .

simonrw commented 4 years ago

Hi again, I'm surprised you got as far as you did! Currently the build system of the crate is strongly tied to pkg-config to find the cfitsio library. On Windows under msys2 I cannot convince it to find the correct packages (even though the build script shells out to pkg-config which should understand environment variables, it does not work). Under regular powershell/cmd.exe I do not have pkg-config installed at all so cannot get it to compile.

I'm interested in knowing more about this problem, and perhaps adding a facility to build on windows easier.

GreatAttractor commented 4 years ago

Here's my usage (it's worked so far, also for bigger things like gtk-rs):

1) install MSYS2

2) install Rust via rustup-init.exe (variant: x86_64-pc-windows-gnu)

3) launch MSYS2 shell via c:\msys64\mingw64.exe, install development toolchain, install CFITSIO (mingw64/mingw-w64-x86_64-cfitsio)

4) perform $ export PATH=/c/Users/.../.cargo/bin:$PATH and proceed to build the project with cargo (still in the MSYS2 shell)

If the above doesn't work for you, I could reinstall everything from scratch and note the steps to be sure.

simonrw commented 4 years ago

While I'm investigating this on my windows VM, can I ask have you tried adding the bindgen feature to your Cargo.toml? E.g.:

[dependencies]
fitsio = { version = "0.15.0", features = ["bindgen"] }

I say this because the fitsio package was originally created with a static conversion (using the command line version of bingen), and the resulting Rust code bundled with fitsio-sys. This therefore makes some assumptions about the host system (particularly macos 64-bit as this the architecture that I originally used).

Including the bindgen feature to your Cargo.tomlmeans that the fitsio-sys bindings are generated at compile time. This does however complicate the building as the crate then depends on llvm which may be fun to get working under msys2.

This should mean that you get exactly the correct bindings for your architecture (i.e. any platform-specific #ifdefs in the cfitsio code will select your architecture).

Also: I presume that compiling a C program that uses cfitsio works correctly?

simonrw commented 4 years ago

Here's my usage (it's worked so far, also for bigger things like gtk-rs):

1. install MSYS2

2. install Rust via `rustup-init.exe` (variant: `x86_64-pc-windows-gnu`)

3. launch MSYS2 shell via `c:\msys64\mingw64.exe`, install development toolchain, install CFITSIO (`mingw64/mingw-w64-x86_64-cfitsio`)

4. perform `$ export PATH=/c/Users/.../.cargo/bin:$PATH` and proceed to build the project with `cargo` (still in the MSYS2 shell)

If the above doesn't work for you, I could reinstall everything from scratch and note the steps to be sure.

I'll have another go when I get the chance (hopefully this evening)

simonrw commented 4 years ago

Ok so I've reproduced the issue. I think you installed the mingw-w64-x86_64-pkg-config package, which I hadn't. Without it, using the default /usr/bin/pkg-config did not find the file correctly, due to windows file path confusion.

I'll take a look at the compilation issue.

GreatAttractor commented 4 years ago

can I ask have you tried adding the bindgen feature to your Cargo.toml?

It starts to download & build a bunch of crates, but eventually fails with:

warning: unused manifest key: dependencies.fitsio.no-default-features
   Compiling fitsio-sys v0.3.0
   Compiling syntex_syntax v0.54.0
error[E0423]: expected function, tuple struct or tuple variant, found struct `ast::Name`
   --> C:\Users\...\.cargo\registry\src\github.com-1ecc6299db9ec823\syntex_syntax-0.54.0\src\symbol.rs:146:27
    |
146 |                       name: ast::Name($index),
    |                             ^^^^^^^^^
...
165 | / declare_keywords! {
166 | |     // Invalid identifier
167 | |     (0,  Invalid,        "")
168 | |
...   |
231 | |     (56, CrateRoot, "{{root}}")
232 | | }
    | |_- in this macro invocation

error: aborting due to previous error

Also: I presume that compiling a C program that uses cfitsio works correctly?

Yes, works fine.

Also, there's no error when using fitsio-sys - I'm guessing the bindings themselves are cross-platform-OK, because they use ::std::os::raw::c_* types, so will match whatever's right on current platform.

The problem comes probably from this kind of statements (e.g. in headers.rs):

#[cfg(target_pointer_width = "64")]
reads_key_impl!(i64, fits_read_key_lng);
#[cfg(target_pointer_width = "32")]
reads_key_impl!(i64, fits_read_key_lnglng);

It seems they should do a different thing on Windows and Unices.

Unfortunately I don't have the time to dig into this at the moment.

simonrw commented 4 years ago

Also, there's no error when using fitsio-sys - I'm guessing the bindings themselves are cross-platform-OK, because they use ::std::os::raw::c_* types, so will match whatever's right on current platform.

The problem comes probably from this kind of statements (e.g. in headers.rs):

#[cfg(target_pointer_width = "64")]
reads_key_impl!(i64, fits_read_key_lng);
#[cfg(target_pointer_width = "32")]
reads_key_impl!(i64, fits_read_key_lnglng);

It seems they should do a different thing on Windows and Unices.

Thanks for this, I have a fix in the works sorting this out. I have also created an issue for the bindgen feature (#119)

simonrw commented 4 years ago

This is proving to be a bigger challenge than I anticipated.

On Windows, a long data type is 32 bits rather than 64 bits. I think this is handled correctly by cfitsio but it is not handled correctly by the fitsio rust package. This will likely require substantial changes to the code base, including conditional handling using cfg checks throughout. This is not something that is quickly solved.

@GreatAttractor: I understand your wish to use fitsio on Windows using msys2. I recognise that Windows is a major platform for astronomy and Rust. I understand that msys2 is a reasonable compromise to getting a *nix-y environment on Windows. I hope you understand that this may take some time to fix.

I'll keep updating this issue with progress.

GreatAttractor commented 4 years ago

Of course, thank you for your effort. Since at the moment I'm using only basic functionality, I should manage with using fitsio-sys directly.

art-den commented 2 years ago

Any chance this will be fixed? I'm searching rust library for fits files and I think cfitsio wrapper is best choice for me )

simonrw commented 2 years ago

Sorry all that I've not revisited this issue for a while. I'm still interested in getting this crate to work under Windows, however I no longer have access to a Windows machine myself. I am rebuilding my environment, so that I can continue this thread. I will revisit this once my VM is up and running.

@art-den are you ok with running this code under msys2, or do you need msvc compatibility? Also you should be able to use fitsio-sys by the sounds of it. This is a direct Rust binding to cfitsio and the API matches accordingly. You could always wrap the library in your own abstraction layer in the short term. Unfortunately this was not a small amount of work, but you're welcome to look at the source of this crate while you wait. You're bound to spot areas where I was naive or that there is a better approach for than I found originally when creating the crate.

simonrw commented 2 years ago

@art-den can you confirm if you would like msvc compatibility or if msys2 support is enough?

art-den commented 2 years ago

@art-den can you confirm if you would like msvc compatibility or if msys2 support is enough?

Do you mean x86_64-pc-windows-msvc rust toolchain? I use msys2 for gcc, pkg-config and libraries with x86_64-pc-windows-gnu rust toolchain.

I will look on fitsio-sys. Also I tested other fits libraries but unfortunately they lack of support with some fits files

simonrw commented 2 years ago

Do you mean x86_64-pc-windows-msvc rust toolchain? I use msys2 for gcc, pkg-config and libraries with x86_64-pc-windows-gnu rust toolchain.

Ok that's great, I'm working on supporting that toolchain at the moment :+1:

simonrw commented 2 years ago

I've created this PR - can those who are interested do the following:

In your Cargo.toml file, include:

fitsio = { git = "https://github.com/mindriot101/rust-fitsio", branch = "win-support" }

and include the dependencies:

Please let me know if you experience any problems, particularly data corruption or invalid reads/writes. If you have the equivalent C code, I'd appreciate a comparison.

cc @art-den @GreatAttractor

art-den commented 2 years ago

I've created this PR - can those who are interested do the following:

Is working for me. Thanks! Will do more tests later

simonrw commented 2 years ago

@art-den how did your tests go? For transparency, the reason I haven't released this feature is (assuming it's working) I'd like to build and test on Windows in the CI pipeline, however it's been a slow process.

art-den commented 2 years ago

First, I tried to build it with 

[dependencies]
fitsio = { git = "https://github.com/mindriot101/rust-fitsio.git", branch = "win-support"  }

and there were no errors. Then I ran several examples from https://docs.rs/fitsio/latest/fitsio/ to read FITS files. I didn't test FITS files writing. So I just made sure 1) no build errors, 2) looks like it works. I write simple astrohotography utility now and plan to use FITS library in it later

art-den commented 2 years ago

No changes? I want to change FITS library from fitrs into rust-fitsio due to license problems. But I still can't compile with

[dependencies]
fitsio = "0.19"
simonrw commented 2 years ago

@art-den Sorry but unfortunately I haven't merged the MR yet. I spent some time trying to get windows CI tests running, but I was not able to. I will probably disable them for now to get this merged in, then add windows CI at a later date. You should expect to see support in 0.20.0

simonrw commented 2 years ago

@art-den can you try with fitsio version 0.20.0? I've just pushed to crates.io

art-den commented 2 years ago

Thanks! With 0.20.0 is 1) compiles, 2) basic functionality (reading ans writing images) is working. I'd switched into rust-fitsio crate