JuliaIO / JLD2.jl

HDF5-compatible file format in pure Julia
Other
537 stars 84 forks source link

compat: no MmapIO for windows 7 #509

Closed HongBinYu-hub closed 7 months ago

HongBinYu-hub commented 7 months ago

Ensure win7 can be precompiled Win7 and below systems use IOStream method to ensure pre-compilation correct.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a0d0730) 85.94% compared to head (499c1f0) 85.92%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #509 +/- ## ========================================== - Coverage 85.94% 85.92% -0.03% ========================================== Files 31 31 Lines 4213 4213 ========================================== - Hits 3621 3620 -1 - Misses 592 593 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

johnnychen94 commented 7 months ago

I really think you should change the default value for iotype (everywhere) to be DEFAULT_IO_TYPE, where

# Windows 7 doesn't support mmap, falls back to IOStream
const DEFAULT_IO_TYPE = if is_win7()
    IOStream
else
    MmapIO
end

is_win7() = Sys.iswindows() && Sys.windows_version().major <= 6 && Sys.windows_version().minor <= 1

The current version doesn't really fix anything: what makes the default and most commonly used jldopen usages broken is still broken.

johnnychen94 commented 7 months ago

@JonasIsensee Are you okay with this compatibility patch? For some reason, we (Suzhou Tongyuan) need to keep compatibility with some "strange" platforms, e.g., windows 7. We prefer to contribute upstream first, so here's the patch.

If you don't want to get this patch in, we understand, and we'll instead start to maintain our internal fork version.

JonasIsensee commented 7 months ago

@JonasIsensee Are you okay with this compatibility patch? For some reason, we (Suzhou Tongyuan) need to keep compatibility with some "strange" platforms, e.g., windows 7. We prefer to contribute upstream first, so here's the patch.

If you don't want to get this patch in, we understand, and we'll instead start to maintain our internal fork version.

I apologize for the late reply.. I like this approach: const DEFAULT_IO_TYPE = if is_win7()

johnnychen94 commented 7 months ago

Thanks for merging!

I'll use my privilege to make a patch release #514