Skip to content

BUG: Force 64-bit storage for xdr file offsets#2024

Merged
jbarnoud merged 1 commit intodevelopfrom
issue_2021_filesize_bytes
Aug 6, 2018
Merged

BUG: Force 64-bit storage for xdr file offsets#2024
jbarnoud merged 1 commit intodevelopfrom
issue_2021_filesize_bytes

Conversation

@tylerjereddy
Copy link
Member

  • Fixes Issue BUG (windows): seeking over 4 GB on xdr formats #2021 with Windows xdr large file
    offset handling; should reduce appveyor failure
    count from 13 -> 11 if no new issues
    recently introduced in dev branch

  • on the Windows platform xdr file offsets are now
    forced to use a 64-bit storage type so that, for example,
    file seeks beyond 4 GB are correctly reported

Fixes #2021

There may be a better way to do this but off64_t wasn't being recognized on my Windows box despite including the type headers.

@codecov
Copy link

codecov bot commented Aug 5, 2018

Codecov Report

Merging #2024 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2024      +/-   ##
===========================================
+ Coverage    88.45%   88.47%   +0.02%     
===========================================
  Files          143      143              
  Lines        17223    17247      +24     
  Branches      2637     2641       +4     
===========================================
+ Hits         15234    15260      +26     
+ Misses        1389     1387       -2     
  Partials       600      600
Impacted Files Coverage Δ
package/MDAnalysis/transformations/rotate.py 100% <0%> (ø) ⬆️
package/MDAnalysis/lib/util.py 86.86% <0%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c9d6c5...fe792a1. Read the comment docs.

@tylerjereddy
Copy link
Member Author

There's something alarming about Windows tests in dev branch (the appveyor merge test) -- someone must have merged something without checking the total failure count on Appveyor. I'm not really sure what to do about this, but I can't outpace the rate of breakage from multiple devs if we don't prevent that somehow.

@jbarnoud
Copy link
Contributor

jbarnoud commented Aug 5, 2018

From the ftello manpage, it seems that xdrio_getpos may return a int32 on some platforms. To be on the safe side, maybe it would be good to set #define _FILE_OFFSET_BITS 64 and make sure we always get an int64? See https://linux.die.net/man/3/ftello.

@jbarnoud
Copy link
Contributor

jbarnoud commented Aug 5, 2018

@tylerjereddy See #2025 for that particular spike in AppVeyor errors. Could we have the AppVeyor build fail if the number of errors is above a given threshold? We can then manually debump the threshold every time a PR fixes a windows failure until the threshold becomes 0. This way, we get a red status which makes AppVeyor much more difficult to ignore.

@richardjgowers
Copy link
Member

richardjgowers commented Aug 5, 2018 via email

@jbarnoud
Copy link
Contributor

jbarnoud commented Aug 5, 2018 via email

* Fixes Issue #2021 with Windows xdr large file
offset handling

* on the Windows platform xdr file offsets are now
forced to use a 64-bit storage type so that, for example,
file seeks beyond 4 GB are correctly reported
@tylerjereddy tylerjereddy force-pushed the issue_2021_filesize_bytes branch from d196f8d to fe792a1 Compare August 5, 2018 17:29
@tylerjereddy
Copy link
Member Author

I revised to add the preprocessor flag @jbarnoud requested; I didn't have success removing my type adjustments after adding that flag though (would have been nice if that worked).

I've also rebased and force pushed: the appveyor merge AND branch tests should both show 11 total test failures (down from 13). There should also be 1 xfail & 9 errors on Windows; I'll look into xfail-tagging as discussed here in a separate PR as time permits.

@jbarnoud jbarnoud self-assigned this Aug 6, 2018
@jbarnoud jbarnoud added this to the 0.19.0 milestone Aug 6, 2018
@jbarnoud jbarnoud merged commit 10e16c0 into develop Aug 6, 2018
@jbarnoud jbarnoud deleted the issue_2021_filesize_bytes branch August 6, 2018 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants