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

introduce ZSTD_USE_C90_QSORT #4312

Merged
merged 3 commits into from
Feb 27, 2025
Merged

introduce ZSTD_USE_C90_QSORT #4312

merged 3 commits into from
Feb 27, 2025

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Feb 21, 2025

in order to compile successfully when qsort_r() isn't supported and this is not detectable at compile time,
such as, for example, older versions of musl.

Fixes #4311

@Cyan4973 Cyan4973 changed the title added musl compilation test in CI introduce ZSTD_USE_C90_QSORT Feb 21, 2025
@Cyan4973 Cyan4973 marked this pull request as ready for review February 21, 2025 20:50
@Cyan4973 Cyan4973 merged commit a1a5154 into facebook:dev Feb 27, 2025
101 checks passed
Copy link

@iucoen iucoen left a comment

Choose a reason for hiding this comment

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

Line 294, 310 should be

#elif defined(_GNU_SOURCE) && defined(ZSTD_USE_C90_QSORT)

Otherwise I don't think this code compiles.

@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Mar 7, 2025

I see your point (though it's missing a !).

Interestingly, there is a musl compilation test in CI that employs this build macro:
https://github.com/facebook/zstd/blob/dev/.github/workflows/dev-short-tests.yml#L696
and it has been running fine so far.
Specifically, it detected the prototype mismatch, but just generated a warning, so the compilation test still completed successfully. And running the binary also works fine.

Anyway, seems like the test needs to be reinforced.

@iucoen
Copy link

iucoen commented Mar 7, 2025

Anyway, seems like the test needs to be reinforced.

It's a warning:

/root/zstd/lib//dictBuilder/cover.c:341:45: warning: passing argument 4 of 'qsort' from incompatible pointer type [-Wincompatible-pointer-types]
  341 |           (ctx->d <= 8 ? &COVER_strict_cmp8 : &COVER_strict_cmp));
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
      |                                             |
      |                                             int (*)(const void *, const void *, void *)
I

Run with -Werror?

@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Mar 7, 2025

Sure, but also the current runtime test would miss the consequences of this signature mismatch (still completes successfully), so it needs an update too.

Cyan4973 added a commit to Cyan4973/zstd that referenced this pull request Mar 11, 2025
and upgraded the test so that it would fail, both at compile time and at run time, without the fix
Cyan4973 added a commit to Cyan4973/zstd that referenced this pull request Mar 11, 2025
and upgraded the test so that it would fail, both at compile time and at run time, without the fix
Cyan4973 added a commit that referenced this pull request Mar 12, 2025
fix #4312 - musl compilation compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not build with musl libc -- qsort_r not available there
4 participants