Skip to content

Conversation

@jorisdral
Copy link
Collaborator

@jorisdral jorisdral commented Jun 19, 2025

This PR prepares blockio for it's first release. Nothing about the implementations changes, but we clean up the public API by refactoring/renaming a little bit, and adding/reworking documentation

@jorisdral jorisdral self-assigned this Jun 19, 2025
@jorisdral jorisdral force-pushed the jdral/blockio-0.1.0.0 branch 7 times, most recently from 71c96a5 to 176473a Compare June 23, 2025 11:00
@jorisdral jorisdral marked this pull request as ready for review June 23, 2025 11:23
@jorisdral jorisdral force-pushed the jdral/blockio-0.1.0.0 branch from 2657ae6 to 54a958f Compare July 1, 2025 10:43
@jorisdral jorisdral force-pushed the jdral/blockio-0.1.0.0 branch from 54a958f to 55d828a Compare July 3, 2025 08:14
Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looks good!

-- even though we'd prefer to keep it truly hidden. There are ways around
-- this, for example using a new private sub-library that contains the
-- "System.FS.BlockIO.Serial" module, but it's a lot of boilerplate.
serialHasBlockIO
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I totally follow why we don't want to export serialHasBlockIO from the public API. Isn't it ok to always have available, in addition to the platform specific/native one?

Copy link
Collaborator Author

@jorisdral jorisdral Jul 4, 2025

Choose a reason for hiding this comment

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

serialHasBlockIO as it stands now has a very unwieldy type:

serialHasBlockIO ::
     (MonadThrow m, MonadMVar m, PrimMonad m, Eq h)
  => (Handle h -> Bool -> m ())
  -> (Handle h -> API.FileOffset -> API.FileOffset -> API.Advice -> m ())
  -> (Handle h -> API.FileOffset -> API.FileOffset -> m ())
  -> (FsPath -> LockMode -> m (Maybe (API.LockFileHandle m)))
  -> (Handle h -> m ())
  -> (FsPath -> m ())
  -> (FsPath -> FsPath -> m ())
  -> HasFS m h
  -> m (API.HasBlockIO m h)
serialHasBlockIO hSetNoCache hAdvise hAllocate tryLockFile hSynchronise synchroniseDirectory createHardLink hfs = do

It's filling in submitIO and close in the resulting HasBlockIO by itself using the input HasFS, but for all the other HasBlockIO functions the user has to provide the implementation. Depending on the platform or simulation, each of these functions is different. I think there would be value in exposing an alternative serialHasBlockIO function with all these HasBlockIO functions filled in, but then those would be exposed from platform-/simulation- specific modules. However, if we move these HasBlockIO functions, that have nothing to do with batched I/O, to fs-api/fs-sim, then our lives become easier because all the unwieldy function arguments disappear and as a result we can just expose serialHasBlockIO from a public module

I say let's keep this function internal for now, and later we can address this (if asked for)

jorisdral added 4 commits July 4, 2025 13:19
…stead

It's specific to the `IO` implementation
We use `serialHasBlockIO` in the `sim` sub-library and so we have to expose it
somewhere, but we'd actually prefer it if it was not part of the public API.
Exposing it only from an internal module makes it clear that it should ideally
not be used by users.
We remove documentation about implementation specifics from the `HasBlockIO`
type, but we'll include it in the `IO` module in one of the next commits.
@jorisdral jorisdral force-pushed the jdral/blockio-0.1.0.0 branch from 55d828a to eeddbfe Compare July 4, 2025 11:59
@jorisdral jorisdral force-pushed the jdral/blockio-0.1.0.0 branch from eeddbfe to 254b95a Compare July 4, 2025 12:44
@jorisdral jorisdral force-pushed the jdral/blockio-0.1.0.0 branch from fd135cf to 359f03c Compare July 7, 2025 10:58
@jorisdral jorisdral enabled auto-merge July 7, 2025 12:05
@jorisdral jorisdral added this pull request to the merge queue Jul 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 7, 2025
@jorisdral jorisdral added this pull request to the merge queue Jul 7, 2025
Merged via the queue into main with commit 783ded7 Jul 7, 2025
59 of 60 checks passed
@jorisdral jorisdral deleted the jdral/blockio-0.1.0.0 branch July 7, 2025 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants