Skip to content
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

Update to mtime.2.0.0 #2166

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Update to mtime.2.0.0 #2166

merged 2 commits into from
Jun 9, 2023

Conversation

patricoferris
Copy link
Contributor

A bit of a WIP PR for #2155 -- the main blockers outside of Irmin are in the Irmin-pack dependencies:

pin-depends: [
  # Metrics Unix may have been unnecessarily constrained in opam-repository
  [ "metrics-unix.dev" "git+https://github.com/mirage/metrics#995eb18d"]
  # Needed by Index
  [ "progress.dev" "git+https://github.com/patricoferris/progress#709e1e2b12f4f74dd2c8823dde274dcd0ac35142" ]
  # Needed by Irmin-pack
  [ "index.dev" "git+https://github.com/patricoferris/index#626e7bd38c7311b484df70dcbed48e28584027a6" ]
]

Happened to bump into this whilst trying to rebase the Eio port which now requires mtime >= 2

@patricoferris patricoferris force-pushed the mtime.2.0.0 branch 2 times, most recently from 72d48af to 08641f0 Compare January 10, 2023 14:08
@@ -88,7 +88,7 @@ let create_pp_seconds examples =
Float.neg_infinity examples
in
let finite_pp =
if absmax >= 60. then fun ppf v -> Mtime.Span.pp_float_s ppf v
if absmax >= 60. then fun ppf v -> Fmt.uint64_ns_span ppf (Int64.of_float v)
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 kinda made this bit up to be honest, not sure what you would like to do here?

Copy link
Member

Choose a reason for hiding this comment

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

Your choice seems reasonable to me since I think it is close to what was there with the previous version of mtime.

@metanivek
Copy link
Member

@patricoferris thanks for doing this set of PRs -- this change in mtime is causing quite the ripple!

I feel conflicted about pushing the lower bounds to 2.0.0 because of the pressure it puts on dependencies, but we will have to if Eio keeps their change and we adopt it.

Either way, looks like we have some upstream dependency shepherding to do before we can merge this.

@metanivek metanivek added the do-not-merge Do not merge this PR label Jan 12, 2023
@patricoferris
Copy link
Contributor Author

Thanks for taking a look @metanivek just wanted to bring it up for when the time comes (if it's a bit noisy feel free to close and just grab the diff at a later date ^^). I had originally started with a set of changes that were backward compatible but it would mean starting to distribute the internals of Mtime to dependencies to preserve the original behaviour I think.

@metanivek
Copy link
Member

I'm happy to keep this open. Added the label just so we don't accidentally merge. 😅

One other point of consideration for us is our downstream consumers, like Tezos. We will need to coordinate this kind of change with them as well.

This is definitely a lesson in the impact of breaking changes for "core" libraries in the ecosystem, like mtime.

Co-authored-by: Kevin Smith <metanivek@gmail.com>
@metanivek metanivek removed the do-not-merge Do not merge this PR label Jun 7, 2023
@metanivek metanivek requested review from art-w and adatario June 7, 2023 14:59
Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

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

Thanks for the initial work on this @patricoferris! I rearranged some code (mainly moving mtime extensions into irmin's import) so we don't have to sprinkle the conversions around.

@art-w @adatario mind giving a second look before merging?

@metanivek metanivek linked an issue Jun 7, 2023 that may be closed by this pull request
Copy link
Contributor

@adatario adatario left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Copy link
Contributor

@art-w art-w left a comment

Choose a reason for hiding this comment

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

Thanks!

@metanivek metanivek merged commit 3222d26 into mirage:main Jun 9, 2023
5 checks passed
metanivek added a commit to metanivek/opam-repository that referenced this pull request Jul 4, 2023
…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.8.0)

CHANGES:

### Added

- **irmin**
  - Change behavior of `Irmin.Conf.key` to disallow duplicate key names by
    default. Add `allow_duplicate` optional argument to override. (mirage/irmin#2252,
    @metanivek)

- **irmin-pack**
  - Add maximum memory as an alternative configuration option, `lru_max_memory`,
    for setting LRU capacity. (@metanivek, mirage/irmin#2254)

### Changed

- **irmin**
  - Lower bound for `mtime` is now `2.0.0` (mirage/irmin#2166, @patricoferris)

- **irmin-mirage-git**
  - Lower bound for `mirage-kv` is now `6.0.0` (mirage/irmin#2256, @metanivek)

### Fixed

- **irmin-cli**
  - Changed `--store irf` to `--store fs` to align the CLI with what is
    published on the Irmin website (mirage/irmin#2243, @wyn)
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.

Update for mtime.2.0.0
4 participants