Skip to content

[core] mv hard-coded zip utils (forked and adapted) to builtins - #22993

Open
ferdymercury wants to merge 1 commit into
root-project:masterfrom
ferdymercury:bzip
Open

[core] mv hard-coded zip utils (forked and adapted) to builtins#22993
ferdymercury wants to merge 1 commit into
root-project:masterfrom
ferdymercury:bzip

Conversation

@ferdymercury

@ferdymercury ferdymercury commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

This Pull request:

Changes or fixes:

This is a C code that was copy-pasted from zip 2.0 and unzip 5.12 in 1993-1994 It was then slightly adapted for ROOT but most of the structure is the same.

Here, the code is put into the builtins directory. This is done for several reasons:

  • consistency with libafterimage, mathtext, glad: they have been all forked from upstream and fixed / adapted for ROOT. Likewise for core/zip, put them in builtins directory to have a better overview of all "dependencies" or borrowed things with upstream counterparts.
  • better separates C code from C++ code, separate folders
  • makes it clearer to deal with licensing and copyright notices by better separating source of the code
  • builtins folder gives kind of more recognition/overview to original authors (before adaptions and modifications)
  • system packagers (eg Debian) might complain less by better clarifying externals vs internals
  • when a bug is found in those builtins, it easens the opportunity to crosscheck with upstream whether it was fixed there
  • it allows in the future a more easy drop-in replacemente with a more up-to-date library or even a real zlib-based builtin rather than a hard-coded copy paste
  • Less annoying surface for clang-tidy etc, things from builtins can be filtered out.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury added the skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff. label Aug 3, 2026
@ferdymercury
ferdymercury marked this pull request as ready for review August 3, 2026 14:22
This is a C code that was copy-pasted from zip 2.0 and unzip 5.12 in 1993-1994
It was then slightly adapted for ROOT but most of the structure is the same.

Here, the code is put into the builtins directory. This is done for several reasons:

- consistency with libafterimage, mathtext, glad: they have been all forked from upstream and fixed / adapted for ROOT. Likewise for core/zip
- better separates C code from C++ code, separate folders
- makes it clearer to deal with licensing and copyright notices by better separating source of the code
- gives more recognition to original authors (before adaptions and modifications)
- when a bug is found, it easens the opportunity to crosscheck with upstream whether it was fixed there
- it allows in the future a more easy drop-in replacemente with a more up-to-date library or even a real zlib-based builtin rather than a hard-coded copy paste
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 15h 41m 42s ⏱️
 3 853 tests  3 852 ✅ 0 💤 1 ❌
78 505 runs  78 496 ✅ 8 💤 1 ❌

For more details on these failures, see this check.

Results for commit 8580b4d.

♻️ This comment has been updated with latest results.

@hageboeck

Copy link
Copy Markdown
Member

Thanks for finding this! Looking at it, I'm wondering if usage of these files can be stopped entirely. When @jblomer is back, we will have a look if we can survive without them.
Would it be OK to wait for the result of the discussion?

@ferdymercury

ferdymercury commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

I'm wondering if usage of these files can be stopped entirely.

Currently it's being used (at least) by net/http/src/THttpCallArg.cxx, io/io/src/TBufferJSON.cxx and graf3d/gl/src/TGLSdfFontMaker.cxx

@linev can maybe comment on this, he recently fixed two bugs on these files

#22988
#22971

Maybe those bugs were no longer there in more-upstream versions of those files.

Would it be OK to wait for the result of the discussion?

Of course, no hurries.

@pcanal

pcanal commented Aug 6, 2026

Copy link
Copy Markdown
Member

Are the failure understood (and/or spurious)? This is a 'core' PR and thus (essentially) requires all test to pass before being merged.

@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label Aug 7, 2026
@ferdymercury ferdymercury reopened this Aug 7, 2026
@ferdymercury

Copy link
Copy Markdown
Collaborator Author

Are the failure understood (and/or spurious)?

yes, they seem known (unrelated) and spurious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clean build Ask CI to do non-incremental build on PR skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants