Skip to content

Add accessors for st_blocks and st_blksize #78

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

Merged
merged 1 commit into from
Feb 15, 2021

Conversation

DanielG
Copy link
Contributor

@DanielG DanielG commented Nov 2, 2016

AFAIU st_blocks is mandatory in POSIX but st_blksize is part of the XSI option (whatever that means) so it's definition is protected by a feature test macro.

This depends on https://ghc.haskell.org/trac/ghc/ticket/12795.

@hvr
Copy link
Member

hvr commented Nov 2, 2016

Fyi, I need to think about the conditional API part; we're trying hard to avoid conditional exports going forward with unix, c.f. #23

@DanielG
Copy link
Contributor Author

DanielG commented Dec 12, 2016

How about this then?

@hvr
Copy link
Member

hvr commented Dec 12, 2016

@DanielG better... otoh, if you know it's conditional, you can also make it into a Maybe, that way it can be caught w/o exception handling

unsafePerformIO $ withForeignPtr stat $ (#peek struct stat, st_blksize)
#else
{-# WARNING fileBlockSize "fileBlockSize: not available on this platform" #-}
fileBlockSize =
Copy link
Member

Choose a reason for hiding this comment

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

btw, this CPP branch is missing its typesig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fine now.

@DanielG
Copy link
Contributor Author

DanielG commented Jul 24, 2018

@hvr what's the status on this? Any reason this never got merged?

@llelf
Copy link
Member

llelf commented Jul 28, 2018

AFAIU st_blocks is mandatory in POSIX

it's not.

@DanielG
Copy link
Contributor Author

DanielG commented Jul 29, 2018

Indeed both st_blocks and st_blksize are XSI options. Thanks @llelf

@Bodigrim
Copy link
Contributor

This is blocked until a) either we drop support of older GHCs, b) or agree our stance on conditional APIs, c) fix backward compatibility in other way.

Copy link
Contributor

@hs-viktor hs-viktor left a comment

Choose a reason for hiding this comment

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

Looks largely fine, but unless I'm missing something about who sees the #WARNINGs about missing optional structure members, those warnings should go.

@@ -250,6 +251,10 @@ specialDeviceID :: FileStatus -> DeviceID
-- | Size of the file in bytes. If this file is a symbolic link the size is
-- the length of the pathname it contains.
fileSize :: FileStatus -> FileOffset
-- | Number of block blocks allocated for this file, in units of 512-byte.
fileBlocks :: FileStatus -> Maybe CBlkCnt
Copy link
Contributor

Choose a reason for hiding this comment

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

The types CBlkCnt and CBlkSize below don't appear to be defined with GHC <= 8.0 breaking CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed these were introduced conditionally in GHC 8.2 and this PR can probably use the corresponding macros to detect their presence. The new feature can then only be offered if the stat structure has the requisite fields (already done) AND the types are available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. Not sure how we missed that before. I'll add some more GLASGOW_HASKELL version checking then.

Copy link
Contributor

Choose a reason for hiding this comment

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

This CPP guard is not sufficient. GHC 8.2 conditionally defines these types. So you need to:

#include "HsBaseConfig.h"

and use the same feature macros.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which means that the types might in fact be unsupported on some platforms even with GHC >= 8.2. However, when that's the case we could define them to be new types for Void in that case, and then the functions can return Maybe X -- X ~ Void, which can only be instantiated as Nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but that doesn't work because HsBaseConfig.h defines a bunch of conflicting defines like PACKAGE_NAME etc. which we also have in HsUnix.h.

I'm pretty sure that if the st_ fields exist the corresponding types must also be defined in the system headers so there shouldn't be any need to check for the types and struct fields separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then the functions can return Maybe X -- X ~ Void

I thought about that too, but then the API is conditional depending on platform factors again :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that if the st_ fields exist the corresponding types must also be defined

Looks like these didn't always exist, at least according to https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html

Issue 5

[...]
The type of st_blksize is changed from long to blksize_t; the type of st_blocks is changed from long to blkcnt_t.

@hs-viktor
Copy link
Contributor

This is blocked until a) either we drop support of older GHCs, b) or agree our stance on conditional APIs, c) fix backward compatibility in other way.

I am not sure why "older GHCs" come into play here. Sure we need at least GHC 8.2 for the types, but since in any case the member fields of "stat" are not universally present, and the PR now returns Nothing on platforms where they're missing, it can (and should) also return Nothing with older GHCs, so the API will be the same API for all systems, but on some systems the return value will always be Nothing. I think that's acceptable.

In other words, given GHC >= 8.2, and a platform that has these fields in struct stat the API will return Just <value> for these fields, and otherwise Nothing. I think this PR can move forward mostly as-is, with appropriate CPP conditionals.

@DanielG DanielG force-pushed the more-stat-fields branch 2 times, most recently from 11b3a8a to ef0cc33 Compare November 17, 2020 18:48
Copy link
Contributor Author

@DanielG DanielG left a comment

Choose a reason for hiding this comment

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

it can (and should) also return Nothing with older GHCs

No it can't because we can't mention the new types since they don't exist on old GHCs yet.

I think there is simply no (good) way to avoid having a conditional API depending on the GHC version. We can however still offer the same API regardless of the availability of the two struct fields. I think that's acceptable.

@@ -250,6 +251,10 @@ specialDeviceID :: FileStatus -> DeviceID
-- | Size of the file in bytes. If this file is a symbolic link the size is
-- the length of the pathname it contains.
fileSize :: FileStatus -> FileOffset
-- | Number of block blocks allocated for this file, in units of 512-byte.
fileBlocks :: FileStatus -> Maybe CBlkCnt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. Not sure how we missed that before. I'll add some more GLASGOW_HASKELL version checking then.

@DanielG
Copy link
Contributor Author

DanielG commented Nov 17, 2020

Changes:

  • Decided to use MIN_VERSION_base instead of GLASGOW_HASKELL as a more direct check for the CBlk* types.

@DanielG
Copy link
Contributor Author

DanielG commented Nov 17, 2020

Changes:

  • Fix typo in fileBlocks docs

@hs-viktor
Copy link
Contributor

Any interest in rebasing this on master, which now has a more comprehensive CI?
This looks rather close now. I'd like to see whether it is possible to get sensible behaviour with GHC 8.0, with the functions returning Nothing :: Maybe SomeIntegralType for some suitable choice of SomeIntegralType.

@hs-viktor
Copy link
Contributor

Please rebase, now that #175 is merged.

@DanielG
Copy link
Contributor Author

DanielG commented Feb 14, 2021

Sure, thanks for the ping :)

@DanielG
Copy link
Contributor Author

DanielG commented Feb 14, 2021

Changes:

  • Remove #ifdefs for MIN_VERSION_base

Copy link

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

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

Looks ready overall, just curious what others make of my strictness question?

Copy link
Contributor

@hs-viktor hs-viktor left a comment

Choose a reason for hiding this comment

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

With FreeBSD passing CI after initially timing out in some sort of odd virtualbox error, this appears to be ready. I'm satisfied with the Just/unsafePerformIO order rationale by @Bodigrim, so this is ready to go. So I'm approving and merging this PR.

@hs-viktor hs-viktor merged commit 9c1619b into haskell:master Feb 15, 2021
@DanielG
Copy link
Contributor Author

DanielG commented Feb 15, 2021

🎉 This has to be my longest running PR by far, hehe, only took a measly 4.25 years to get merged, thanks everyone :D

@vdukhovni
Copy link

Congratulations! Now that we've sorted out the CI, and dropped support for old GHC versions that can make it difficult to adopt new features, it should be possible to address the backlog without similar delays.

@Bodigrim Bodigrim modified the milestones: 2.7.3.0, 2.8.0.0 Apr 20, 2022
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.

6 participants