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

Fix zstd-dll build missing dependencies #3496

Merged
merged 3 commits into from
Feb 12, 2023
Merged

Conversation

yoniko
Copy link
Contributor

@yoniko yoniko commented Feb 10, 2023

Fixes zstd-dll build (#3492 ):

  • Adds pool.o and threading.o dependency to the zstd-dll target
  • Moves custom allocation functions into header to avoid needing to add dependency on common.o
  • Adds test target for zstd-dll
  • Adds github workflow for test zstd-dll

@@ -24,6 +24,8 @@
#include "mem.h"
#include "debug.h" /* assert, DEBUGLOG, RAWLOG, g_debuglevel */
#include "error_private.h"
#define ZSTD_DEPS_NEED_MALLOC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Cyan4973 - Is there any issue with adding this to zstd_internal.h?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, zstd_internal.h is used everywhere within libzstd, so this change can impact a lot of places.
I'm not sure if I can understand all the implications.

@yoniko yoniko force-pushed the fix-zstd-dll-build branch 3 times, most recently from 971fb78 to 45f9c60 Compare February 10, 2023 22:34
@Cyan4973
Copy link
Contributor

This PR manages to get zstd-dll recipe working by adding 2 units, threading.c and pool.c.
This is an improvement over a naive solution which requires 4 units, including zstd_common.c and error_private.c.

To achieve that goal, this PR moves customMalloc() and co from zstd_common.c to zstd_internal.h.
This operation requires making zstd_deps.h a dependency of zstd_internal.h, and therefore, transitively, a dependency of almost all source files inside libzstd.

Now, I'm not exactly pleased about this dependency story.
But on the other hand, I don't think we lose too much in this PR specifically.

zstd_common.c is not terribly specific,
moving functions from there to zstd_internal.h doesn't improve the situation, but it does not make it significantly worse either.

Bundling more definitions within zstd_internal.h will transitively increase compilation load time of all units depending on it. But the increased workload is likely very light, so it's unlikely to have a big impact.

All in all, it seems this is an acceptable trade off.

Longer term, I'm not pleased to see zstd_internal.h employed as a "bag of everything" which transitively includes a ton of other stuffs. Initially, this file was designed to hold common information that should not be duplicated between compression and decompression, such as the Frame ID number. It has largely deviated from this original scope, and I think this doesn't respect the libzstd model of minimal dependency at each step. Hence, I'll be pleased to see this bundle gradually separated into more specific packets that are included only when needed.

But that's a topic for a different effort, and since this PR is focused on zstd-dll, it wouldn't be fair to address that here.

- Adds pool.o and threading.o dependency to the zstd-dll target
- Moves custom allocation functions into header to avoid needing to add dependency on common.o
- Adds test target for zstd-dll
- Adds github workflow for test zstd-dll
@yoniko yoniko force-pushed the fix-zstd-dll-build branch 3 times, most recently from acb7257 to 2b44b8c Compare February 11, 2023 00:49
@yoniko yoniko changed the title Fixes zstd-dll build Fix zstd-dll build missing dependancies Feb 11, 2023
@yoniko yoniko changed the title Fix zstd-dll build missing dependancies Fix zstd-dll build missing dependencies Feb 11, 2023
@yoniko
Copy link
Contributor Author

yoniko commented Feb 11, 2023

@Cyan4973 - all tests pass. Please see most recent changes, if it looks good to you we can merge.

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.

3 participants