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

modules/zstd: build libzstd #1166

Merged
merged 3 commits into from
Nov 24, 2023
Merged

Conversation

rdob-ant
Copy link
Contributor

This PR adds zstd library to the XLS framework. It exposes the following targets:

@com_github_facebok_zstd//:libzstd - zstd library itself
@com_github_facebok_zstd//:zstd - binary for performing zstd compression/decompression
@com_github_facebok_zstd//:decodecorpus - binary for creating test data for a zstd decoder

Additionally, we remove zstd from llvm build to avoid linking conflicts with newly-added zstd library. It seems that llvm_zstd is not necessary for correct llvm operation.

@proppy
Copy link
Member

proppy commented Oct 26, 2023

@com_github_facebok_zstd//:zstd - binary for performing zstd compression/decompression
@com_github_facebok_zstd//:decodecorpus - binary for creating test data for a zstd decoder

Do we need to those? It sounds like we would motsly need to drive (software)-zstd from C++ tests to compare data with DSLX/IR simulation (and not from the shell or bazel rules).

@rw1nkler
Copy link
Contributor

rw1nkler commented Oct 30, 2023

They were added for our convenience during development but we can remove those two targets from the build file. Later, we will extend the build rules to expose internal ZSTD headers because we need them for more advanced tests.

@proppy proppy self-requested a review November 9, 2023 13:43
Copy link
Member

@proppy proppy left a comment

Choose a reason for hiding this comment

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

can you squash the commits, and rename the PR (without the part 1)

@proppy proppy changed the title modules/zstd: build libzstd (part 1) modules/zstd: build libzstd Nov 9, 2023
@proppy
Copy link
Member

proppy commented Nov 9, 2023

also if you rebase, that should get rid of the macosx continuous integration failure which is already fixed at HEAD (and it only runs nightly now)

@@ -286,3 +286,11 @@ def load_external_repositories():
"https://github.com/nlohmann/json/archive/refs/tags/v3.10.2.tar.gz",
],
)

http_archive(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment saying the date this was fresh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added comment with release info and dependency update date

],
strip_include_prefix = "lib",
linkopts = [
"-pthread",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? I'd have thought not

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like disabling multithread builds does not affect our tests which use zstd library. I've updated the build rule to build library without multithread support.

@lpawelcz lpawelcz force-pushed the 49966-zstd-cleaned branch 2 times, most recently from f364507 to 75a6a22 Compare November 23, 2023 11:21
rw1nkler and others added 3 commits November 23, 2023 12:24
This commit enables building zstd library in version 1.4.7 in
the XLS repository. The sources of the library come from
the official release of zstd on Meta's GitHub.

Internal-tag: #[49966]
Signed-off-by: Robert Winkler <[email protected]>
Internal-tag: [#49966]
Signed-off-by: Pawel Czarnecki <[email protected]>
Disable building llvm with zstd to avoid conflicts with newly-added zstd
library. It seems that llvm_zstd is not necessary for correct llvm
operation.

Internal-tag: #[49966]
Signed-off-by: Robert Winkler <[email protected]>
@copybara-service copybara-service bot merged commit 75dbac2 into google:main Nov 24, 2023
4 checks passed
@lpawelcz lpawelcz deleted the 49966-zstd-cleaned branch November 27, 2023 06:58
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.

5 participants