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.

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

feature: add KiB,MiB,GiB and TiB values #8618

Merged
merged 1 commit into from
Oct 10, 2023
Merged

feature: add KiB,MiB,GiB and TiB values #8618

merged 1 commit into from
Oct 10, 2023

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Sep 9, 2023 •
edited
Loading

We make these the default printed values when displaying bytes. However the dune lang decoder can understand binary and decimal byte units.

We also expand the test suite to account for parsing and displaying these values.

  • changelog
  • update documentation in cmds
OPTIONS
      --size=BYTES
             Size to trim the cache to. BYTES is the number of bytes followed
             by a unit. Byte units can be one of B, kB, KiB, MB, MiB, GB, GiB,
             TB or TiB.  

       --trimmed-size=BYTES
           Size to trim from the cache. BYTES is the same as for --size.
  • allow 123 to be parsed as 123B or remove test. not so easy to do
  • Should we default to printing binary units or decimal units? printing only decimal like before now

cc @snowleopard

@Alizter Alizter marked this pull request as draft September 9, 2023 15:50
@emillon
Copy link
Collaborator

emillon commented Sep 11, 2023

I'm out of the loop on this change but I don't think we should change the default (controversial change) in the same change we're adding new accepted prefix (less controversial change).

@Alizter
Copy link
Collaborator Author

Alizter commented Sep 11, 2023 •
edited
Loading

@emillon I agree, I will change it back. The main benefit this brings is that we can parse those units too. In the future we may wish to print information about bytes and having the choice of units to use could be useful.

The motivation for this change is the CR that was in bytes_unit. Also, many tools use binary units for downloads and decimal units for storage.

@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch 2 times, most recently from d28dd6c to e24498f Compare September 18, 2023 12:30
Alizter
Alizter commented Sep 18, 2023
View reviewed changes
bin/cache.ml Outdated Show resolved Hide resolved
@Alizter Alizter marked this pull request as ready for review September 18, 2023 12:31
@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch 2 times, most recently from 43af69f to 36b9d51 Compare September 18, 2023 13:42
Alizter
Alizter commented Sep 18, 2023
View reviewed changes
bin/cache.ml Outdated Show resolved Hide resolved
@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch 3 times, most recently from fe1b01e to 9114e2a Compare September 25, 2023 09:54
@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch from 9114e2a to c82f19d Compare October 2, 2023 19:02
@Alizter
Copy link
Collaborator Author

Alizter commented Oct 2, 2023

My main motivation for this change was to have nice download progress indicators in package management. Typically downloads in package managers that I've seen show the units in MiB. This is the reason I bothered with this entire PR.

From the point of view of trimming the cache, this change is kind of useless. So maybe its best to close this for now and resurrect if its actually needed.

@snowleopard
Copy link
Collaborator

snowleopard commented Oct 2, 2023 •
edited
Loading

I think adding units is clearly useful, and if that was the only change that this PR was attempting, it would have landed long ago. (Yet we keep being distracted by other things.)

@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch 3 times, most recently from 6b3f440 to 1004ed0 Compare October 3, 2023 03:34
@Alizter
Copy link
Collaborator Author

Alizter commented Oct 3, 2023

@snowleopard Sorry for the back and forth, it was not my intention to make this difficult to review. I've removed the unrelated parts and the PR now only consists of adding the new units and adding tests testing all of them.

@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch 2 times, most recently from 1bcf491 to e0d7970 Compare October 3, 2023 03:57
@Alizter Alizter mentioned this pull request Oct 4, 2023
Open
@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch from e0d7970 to 6f4560c Compare October 9, 2023 10:56
snowleopard
snowleopard approved these changes Oct 9, 2023
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me now!

@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch from 6f4560c to c69aa2f Compare October 9, 2023 22:41
@Alizter
Copy link
Collaborator Author

Alizter commented Oct 9, 2023

@rgrinberg This is ready to merge now.

@Alizter
dune_lang: add KiB,MiB,GiB and TiB values
bc82e7e 
We make these the default printed values when displaying bytes. However
the dune lang decoder can understand binary and decimal byte units.

We also expand the test suite to account for parsing and displaying these
values.

Signed-off-by: Ali Caglayan <[email protected]>
@Alizter Alizter force-pushed the ps/branch/dune_lang__add_kib_mib_gib_and_tib_values branch from c69aa2f to bc82e7e Compare October 10, 2023 00:50
@rgrinberg rgrinberg changed the title dune_lang: add KiB,MiB,GiB and TiB values feature: add KiB,MiB,GiB and TiB values Oct 10, 2023
@rgrinberg rgrinberg merged commit a98bf2c into ocaml:main Oct 10, 2023
20 checks passed
@emillon emillon mentioned this pull request Nov 28, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Nov 28, 2023
@emillon
[new release] dune (15 packages) (3.12.0)
d595c99 
CHANGES:

- Introduce `$ dune ocaml doc` to open and browse documentation. (ocaml/dune#7262, fixes
  ocaml/dune#6831, @EmileTrotignon)

- `dune cache trim` now accepts binary byte units: `KiB`, `MiB`, etc. (ocaml/dune#8618,
  @Alizter)

- No longer force colors for OCaml 4.03 and 4.04 (ocaml/dune#8778, @rgrinberg)

- Introduce new experimental odoc rules (ocaml/dune#8803, @jonjudlam)

- Introduce the `runtest_alias` field to the `cram` stanza. This allows
  removing default `runtest` alias from tests. (@rgrinberg, ocaml/dune#8887)

- Do not ignore libraries named `bigarray` when they are defined in conjunction
  with OCaml 5.0 (ocaml/dune#8902, fixes ocaml/dune#8901, @rgrinberg)

- Dependencies in the copying sandbox are now writeable (ocaml/dune#8920, @rgrinberg)

- Absent packages shouldn't prevent all rules from being loaded (ocaml/dune#8948, fixes
  ocaml/dune#8630, @rgrinberg)

- Correctly determine the stanza of menhir modules when `(include_subdirs
  qualified)` is enabled (@rgrinberg, ocaml/dune#8949, fixes ocaml/dune#7610)

- Display cache location in Dune log (ocaml/dune#8974, @nojb)

- Re-run actions whenever `(expand_aliases_in_sandbox)` changes (ocaml/dune#8990,
  @rgrinberg)

- Rules that only use internal dune actions (`write-file`, `echo`, etc.) can
  now be sandboxed. (ocaml/dune#9041, fixes ocaml/dune#8854, @rgrinberg)

- Do not re-run rules when their location changes (ocaml/dune#9052, @rgrinberg)

- Correctly ignore `bigarray` on recent version of OCaml (ocaml/dune#9076, @rgrinberg)

- Add `test_` prefix to default test name in `dune init project` (ocaml/dune#9257, fixes
  ocaml/dune#9131, @9sako6)

- Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting
  of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter)

- [coq rules] Be more tolerant when coqc --print-version / --config don't work
  properly, and fallback to a reasonable default. This fixes problems when
  building Coq projects with `(stdlib no)` and likely other cases. (ocaml/dune#8966, fix
  ocaml/dune#8958, @Alizter, reported by Lasse Blaauwbroek)

- Dune will now run at a lower framerate of 15 fps rather than 60 when
  `INSIDE_EMACS`. (ocaml/dune#8812, @Alizter)

- dune-build-info: when `version=""` is found in a `META` file, we now return
  `None` as a version string (ocaml/dune#9177, @emillon)

- Dune can now be built and installed on Haiku (ocaml/dune#8795, fix ocaml/dune#8551, @Alizter)

- Mark installed directories in `dune-package` files. This fixes `(package)`
  dependencies against packages that contain such directories. (ocaml/dune#8953, fixes
  ocaml/dune#8915, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snowleopard snowleopard snowleopard approved these changes

@emillon emillon emillon left review comments

@rgrinberg rgrinberg rgrinberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Alizter @emillon @snowleopard @rgrinberg