JuliaAstro / FITSIO.jl

Flexible Image Transport System (FITS) file support for Julia
http://juliaastro.org/FITSIO.jl/
MIT License
55 stars 29 forks source link

Windows support #42

Closed kbarbary closed 9 years ago

kbarbary commented 9 years ago

@tkelman's JuliaCon talk inspired me to start supporting Windows. In this case, it should be pretty easy because there are pre-built windows binaries for cfitsio on the cfitsio homepage.

I don't expect this PR to work immediately, as I've probably got the BinDeps build script wrong. This is mainly to try AppVeyor out.

Quick question for Tony: The cfitsio homepage provides both MSVC and Borland binaries. Do we have to worry about using the later? (I have no experience with Windows!)

tkelman commented 9 years ago

Didn't know anyone still uses Borland? I took a look and the Borland dll is 32 bit only, and depends on a cc3250mt.dll which I guess must be the Borland compiler's C runtime, which isn't included in the same dll. Try the MSVC builds first. There can in some cases be issues with Julia calling into MSVC-built dll's, it depends exactly what the dll is doing. Can work fine in some cases, worth trying first since cfitsio is not already available on opensuse build service.

You probably need a @windows_only libfilename = "cfitsio.dll" so the variable is defined (even though the BuildProcess provider does not get used by default on Windows). And may need to do libcfitsio = library_dependency("libcfitsio", aliases=["cfitsio"]) too.

kbarbary commented 9 years ago

Thanks Tony.

tkelman commented 9 years ago

Apparently my powershell download example doesn't like FTP? There are a few alternatives at http://www.appveyor.com/docs/how-to/download-file - the powershell ones will need the script line to start with ps:

tkelman commented 9 years ago

Wait, no, that's a bug I need to fix in BinDeps. Will see what I can do.

kbarbary commented 9 years ago

Note that I haven't yet been able to verify that the file actually exists on the server.

tkelman commented 9 years ago

Oh, the link I clicked on the website was version 3370. I can download that ftp url from powershell on my machine, so I think it would work. If you want to be cautious about changing the version used on osx and linux, maybe try hardcoding the windows version?

kbarbary commented 9 years ago

Yeah sorry for the noise on this. I assumed (hoped) that they would keep older versions around, but indeed they do not. That will be annoying; whenever a new version of cfitsio is released, existing FITSIO.jl versions will break (on windows). We'll see if the MSVC binary works at all though.

tkelman commented 9 years ago

We have a mirroring server at https://github.com/staticfloat/cache.julialang.org that we could probably add this to.

tkelman commented 9 years ago

Looks like the zip file isn't unpacking into a separate subfolder, so you probably need to set an unpacked_dir option - possibly to "" or "." or "/" ? Example case here to a custom subdir https://github.com/JuliaOpt/SCS.jl/blob/7c25e72ef01702598e51aeeb0ce8bb68337926e9/deps/build.jl#L30 can hopefully also be made to work in the no-subdir case

tkelman commented 9 years ago

A forward slash works for me locally, but I get this

julia> Pkg.test("FITSIO")
INFO: Testing FITSIO
WARNING: ccall Ptr argument types must now match exactly, or be Ptr{Void}.
 in fits_read_pix at C:\Users\Tony\.julia\FITSIO\src\libcfitsio.jl:554
 in fits_read_pix at C:\Users\Tony\.julia\FITSIO\src\libcfitsio.jl:564
 in read at C:\Users\Tony\.julia\FITSIO\src\image.jl:62
 in anonymous at no file:19
 in include at boot.jl:245
 in include_from_node1 at loading.jl:128
 in process_options at client.jl:285
 in _start at client.jl:354
ERROR: bad first element number
 in error at error.jl:21 (repeats 2 times)
while loading C:\Users\Tony\.julia\FITSIO\test\runtests.jl, in expression starting on line
 11

The problem happens at the read(f[end]) line, for T=Uint8:

julia> f[end]
File: C:\Users\Tony\AppData\Local\Temp\jul409E.tmp.fits
HDU: 1
Type: Image
Datatype: Uint8
Datasize: (5,20)

maybe relevant, I also get

julia> f = FITS(fname, "w")
Error showing value of type FITS:
ERROR: Reducing over an empty array is not allowed.
 in _mapreduce at reduce.jl:160
 in show at C:\Users\Tony\.julia\FITSIO\src\fits.jl:67
 in anonymous at show.jl:1160
 in with_output_limit at show.jl:1137
 in showlimited at show.jl:1159
 in writemime at replutil.jl:2
 in display at REPL.jl:117
 in display at REPL.jl:120
 in display at multimedia.jl:149
 in print_response at REPL.jl:139
 in print_response at REPL.jl:124
 in anonymous at REPL.jl:586
 in run_interface at LineEdit.jl:1379
 in run_frontend at REPL.jl:817
 in run_repl at REPL.jl:169
 in _start at client.jl:400

a little earlier on

kbarbary commented 9 years ago

Thanks for testing this out Tony.

I think the first bug has is due to passing a Vector{Int64} to a C fuction expecting a Ptr{Clong}. Just happens to work when Clong is Int64. The last thing is also a real bug that I also see on linux. Will fix these.

kbarbary commented 9 years ago

unpacked_dir="/" didn't seem to work on AppVeyor.

tkelman commented 9 years ago

That is odd. I think I only tried 64 bit. Did you enable "fast fail" in the settings, or maybe is that the default?

kbarbary commented 9 years ago

No, I cancelled the last build by hand because I was watching it live. win32 finished with a fail, win64 had reached the same failure so I cancelled it.

tkelman commented 9 years ago

Ah yes, I see that now. Apparently I lied to you, or BinDeps lied to me, and "/" only sort of worked once? Or something odd was happening. So there is probably a bug/necessary new feature with BinDeps to make no-subfolder extraction work correctly. There are some sort of ugly things you can do that might work, on the order of how NLopt extracts itself (written before unpacked_dir existed for Binaries): https://github.com/JuliaOpt/NLopt.jl/blob/093fb2030b566ae32b363c277a9460d9719a9e4e/deps/build.jl#L28-L52

kbarbary commented 9 years ago

Thanks, the NLOpt example looks apt. I'll give it a go.

kbarbary commented 9 years ago

With the explicit BuildProcess on Windows, I still get a failure. It makes it past the 7z extraction now, but doesn't find the file in the expected place. Here's a clip from the AppVeyor log:

INFO: Attempting to Create directory C:\Users\appveyor\.julia\v0.3\FITSIO\deps\src\cfitsio.dll

7-Zip [64] 9.20  Copyright (c) 1999-2010 Igor Pavlov  2010-11-18

Processing archive: C:\Users\appveyor\.julia\v0.3\FITSIO\deps\downloads\cfitsio_MSVC_64bit_DLL_3370.zip

Extracting  cfitsio.dll
Extracting  cfitsio.lib
Extracting  FPack.exe
Extracting  Funpack.exe
Extracting  TestProg.exe
Extracting  fpackguide.pdf
Extracting  fitsio.h
Extracting  License.txt
Extracting  longnam.h
Extracting  README.win
Extracting  testprog.std
Extracting  testprog.tpt
Extracting  testprog.out
Extracting  testprog.c

Everything is Ok

Files: 14
Size:       2161783
Compressed: 1078860
===============================[ ERROR: FITSIO ]================================

Directory C:\Users\appveyor\.julia\v0.3\FITSIO\deps\src\cfitsio.dll was not created successfully (Tried to run `7z x 'C:\Users\appveyor\.julia\v0.3\FITSIO\deps\downloads\cfitsio_MSVC_64bit_DLL_3370.zip' -y '-oC:\Users\appveyor\.julia\v0.3\FITSIO\deps\src'` )
while loading C:\Users\appveyor\.julia\v0.3\FITSIO\deps\build.jl, in expression starting on line 55

@tkelman Should the -o in the 7z command be outside the quotes? e.g.

... -o 'C:\Users\...'

rather than

... '-oC:\Users\...'
tkelman commented 9 years ago

I think the quoting is fine, this is able to extract fine on my machine, but the error Directory C:\Users\appveyor\.julia\v0.3\FITSIO\deps\src\cfitsio.dll was not created successfully makes me think you might be giving the arguments in the wrong order to FileUnpacker?

kbarbary commented 9 years ago

I think the order is correct. The error message says "Directory" instead of "File" because this line in BinDeps has a DirectoryRule that should be a FileRule (in that function, target is a file that should exist once the archive is unpacked).

kbarbary commented 9 years ago

PR for above fix: JuliaLang/BinDeps.jl#153

tkelman commented 9 years ago

On that branch of BinDeps, I get

INFO: Testing FITSIO
ERROR: test error during read(f[end],:,:) == indata
`Array{T,N}` has no method matching Array{T,N}(::Type{Uint8}, ::(Int32,Int32))
 in read_internal at C:\Users\Tony\.julia\FITSIO\src\image.jl:117
 in read at C:\Users\Tony\.julia\FITSIO\src\image.jl:124
 in anonymous at test.jl:62
 in do_test at test.jl:37
 in anonymous at no file:24
 in include at boot.jl:245
 in include_from_node1 at loading.jl:128
 in process_options at client.jl:285
 in _start at client.jl:354
while loading C:\Users\Tony\.julia\FITSIO\test\runtests.jl, in expression starting on line
 11
kbarbary commented 9 years ago

Progress! Will investigate.

tkelman commented 9 years ago

You'd have to add a Pkg.checkout("BinDeps") in the appveyor script here, before Pkg.build("FITSIO") until a new release gets tagged. I'm still at MIT so don't have my SSH keys for metadata handy.

kbarbary commented 9 years ago

And we have a pass on Windows 32-bit & 64-bit with Julia 0.3!

Julia 0.4-dev is now failing due to an upstream change. fixing...

kbarbary commented 9 years ago

squashed commits.

tkelman commented 9 years ago

Awesome! See, not an impossible problem.