Skip to content

Fix zstd-dll build missing dependencies#3496

Merged
yoniko merged 3 commits intofacebook:devfrom
yoniko:fix-zstd-dll-build
Feb 12, 2023
Merged

Fix zstd-dll build missing dependencies#3496
yoniko merged 3 commits intofacebook:devfrom
yoniko:fix-zstd-dll-build

Conversation

@yoniko
Copy link
Contributor

@yoniko yoniko commented Feb 10, 2023 •
edited
Loading

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

yoniko
yoniko commented Feb 10, 2023
View reviewed changes
lib/common/zstd_internal.h Outdated
#include "mem.h"
#include "debug.h" /* assert, DEBUGLOG, RAWLOG, g_debuglevel */
#include "error_private.h"
#define ZSTD_DEPS_NEED_MALLOC
Copy link
Contributor Author

@yoniko yoniko Feb 10, 2023

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

@Cyan4973 Cyan4973 Feb 10, 2023

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

Cyan4973 commented Feb 10, 2023

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.

@yoniko
Fixes zstd-dll build (facebook#3492):
6733eaa 
- 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 force-pushed the fix-zstd-dll-build branch from 2b44b8c to 65d628d Compare February 11, 2023 00:59
@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.

@yoniko yoniko merged commit c78f434 into facebook:dev Feb 12, 2023
@yoniko yoniko mentioned this pull request Feb 12, 2023
Closed
@yoniko yoniko mentioned this pull request Mar 15, 2023
Closed
@Cyan4973 Cyan4973 mentioned this pull request Apr 1, 2023
Merged
@MOHAMED19OS MOHAMED19OS mentioned this pull request Apr 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yoniko @Cyan4973 @facebook-github-bot