Skip to content

Extract fileystem API (fs-api) and simulated file system (fs-sim) from ouroboros-consensus and ouroboros-consensus-test. #4425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 15, 2023
Merged

Extract fileystem API (fs-api) and simulated file system (fs-sim) from ouroboros-consensus and ouroboros-consensus-test. #4425

merged 6 commits into from
Mar 15, 2023

Conversation

@jorisdral
Copy link
Contributor

@jorisdral jorisdral commented Mar 3, 2023 •
edited
Loading

Description

Part of #4082.

This PR extracts the filesystem API and simulated filesystem from the the ouroboros-consensus and ouroboros-consensus-test packages. The packages are called fs-api and fs-sim respectively. This PR paves the way for publishing both fs-api and fs-sim on CHaP and Hackage.

See the commit messages for more info about the changes introduced by this PR.

Follow-up PRs:

  • Remove the nothunks (and possibly deepseq) dependencies from fs-api and fs-sim.
  • Remove System.IO.Seek from fs-api
  • Rename fs-api and fs-sim to cardano-fs-api and cardano-fs-sim respectively for releases to CHaP.
  • How should test utilities for io-sim be exposed? In a separate package?
  • Add content to the READMEs for fs-sim and fs-api.
  • Add module header documentation to fs-sim and fs-api.
  • Re-export most important parts of fs-api and fs-sim in a top-level module like System.FS and System.FS.Sim.
  • Add re-exports of System.FS.Types to fs-api modules.

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • The documentation has been properly updated
    • New tests are added if needed and existing tests are updated
    • Any changes affecting Consensus packages must have an entry in the appropriate changelog.d directory created using scriv. If in doubt, see the Consensus release process.
    • Any changes in the Consensus API (every exposed function, type or module) that has changed its name, has been deleted, has been moved, or altered in some other significant way must leave behind a DEPRECATED warning that notifies downstream consumers.
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@jorisdral jorisdral self-assigned this Mar 3, 2023
@jorisdral jorisdral force-pushed the jdral/4082-publish-fs-sim branch 19 times, most recently from e02047d to 5865d7f Compare March 9, 2023 16:29
jorisdral
jorisdral commented Mar 9, 2023
View reviewed changes
fs-sim/src/System/FS/Sim/Error.hs
Comment on lines 498 to 539
fsVar <- uncheckedNewTVarM mockFS
errorsVar <- uncheckedNewTVarM errors
fsVar <- newTVarIO mockFS
errorsVar <- newTVarIO errors
Copy link
Contributor Author

@jorisdral jorisdral Mar 9, 2023

Choose a reason for hiding this comment

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

uncheckedNewTVarM is just an alias for newTVarIO

@jorisdral jorisdral marked this pull request as ready for review March 9, 2023 16:33
@jorisdral jorisdral requested review from a team and removed request for coot, dnadales and nfrisby March 9, 2023 16:33
jorisdral
jorisdral commented Mar 9, 2023
View reviewed changes
jasagredo
jasagredo approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@jasagredo jasagredo 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! thanks!

@jorisdral jorisdral force-pushed the jdral/4082-publish-fs-sim branch from 51c955e to 3f09e91 Compare March 10, 2023 11:12
@dnadales dnadales requested a review from coot March 10, 2023 12:26
@coot
Copy link
Collaborator

coot commented Mar 10, 2023

Related to #4082.

dnadales
dnadales approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

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

This is great Joris 💪

@jorisdral jorisdral force-pushed the jdral/4082-publish-fs-sim branch 2 times, most recently from dbe8eed to fff21ff Compare March 10, 2023 15:29
@jorisdral
Copy link
Contributor Author

jorisdral commented Mar 10, 2023

@jasagredo @dnadales I've added a new commit that leaves behind deprecation warnings in the old locations of the fs-api and fs-sim modules, could you verify that they are correct?

@jorisdral jorisdral force-pushed the jdral/4082-publish-fs-sim branch 2 times, most recently from 9891d03 to d05d476 Compare March 10, 2023 15:52
jorisdral
jorisdral commented Mar 13, 2023
View reviewed changes
@jorisdral
Copy link
Contributor Author

jorisdral commented Mar 13, 2023 •
edited
Loading

We should wait with merging into master until #4448 is merged, since #4425 makes breaking changes in both ouroboros-consensus and ouroboros-consensus-test, which will live in separate package bundles after #4448 is merged.

@jorisdral jorisdral added the consensus issues related to ouroboros-consensus label Mar 13, 2023
@jorisdral
Extract fs-api and fs-sim packages from consensus (test) packages.
418601e 
The `fs-api` package contains the `HasFS` interface to the file system,
and it provides the `HasFS` implementation for the `IO` file system.

The `fs-sim` package contains the `HasFS` implementation for a simulated
file system, together with some test utilities. We'll have to
investigate whether the test utilities should live in a separate lib.

Some utility modules from `ouroboros-consensus` are copied into the
packages and adapated, because we can not depend on
`ouroboros-consensus`, since that would introduce circular dependencies.
In some other cases where dependencies on definitions from utility
modules were only minor, those definitions were inlined or copied from
the utility modules into the `fs-api` and `fs-sim` code.

This commit removes the core `fs-api` and `fs-sim` from
`ouroboros-consensus` and `ourobors-consensus-test`, but does not yet
fix the compilation errors that arise in the `ouroboros-consensus*`
packages. A future commit should make the `ouroboros-consensus-*`
packages depend on `fs-api` and `fs-sim`, and fix the all the
compilation errors.
@jorisdral
Fix compilation for ouroboros-consensus-* packages
8dab74f
@jorisdral
Better version bounds on base for fs-sim-test
5a048a0
@jorisdral
Process PR comments
2a86657
@jorisdral
Add deprecation warnings, add formatting for fs-api and fs-sim
9d847c0
@jorisdral jorisdral force-pushed the jdral/4082-publish-fs-sim branch 2 times, most recently from 098af5d to d7430e5 Compare March 15, 2023 10:26
@jorisdral jorisdral force-pushed the jdral/4082-publish-fs-sim branch from d7430e5 to bf768fd Compare March 15, 2023 12:04
@jorisdral jorisdral mentioned this pull request Mar 15, 2023
Closed
@jorisdral
Copy link
Contributor Author

jorisdral commented Mar 15, 2023

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 15, 2023

Build succeeded:

@iohk-bors iohk-bors bot merged commit 0659e7b into master Mar 15, 2023
@iohk-bors iohk-bors bot deleted the jdral/4082-publish-fs-sim branch March 15, 2023 15:39
@jorisdral jorisdral mentioned this pull request Mar 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnadales dnadales dnadales approved these changes

@jasagredo jasagredo jasagredo approved these changes

@coot coot Awaiting requested review from coot

Assignees

@jorisdral jorisdral

Labels

consensus issues related to ouroboros-consensus enhancement New feature or request open-source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jorisdral @coot @dnadales @jasagredo