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

zstd: Add zstd_cli cc_binary #1730

Merged
merged 4 commits into from
Apr 4, 2024
Merged

zstd: Add zstd_cli cc_binary #1730

merged 4 commits into from
Apr 4, 2024

Conversation

afq984
Copy link
Contributor

@afq984 afq984 commented Apr 1, 2024

No description provided.

@bazel-io
Copy link
Member

bazel-io commented Apr 1, 2024

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (zstd) have been updated in this PR. Please review the changes.

@afq984 afq984 marked this pull request as ready for review April 1, 2024 09:33
@afq984
Copy link
Contributor Author

afq984 commented Apr 1, 2024

This should be the minimal changes to allow building the zstd binary.

However I'd like to have our BUILD.bazel files follow the zstd upstream BUCK files in structure so we don't deviate too much (say, @zstd//programs:zstd instead of @zstd//:zstd_cli), at the cost of bumping the compatibility level. Thoughts?

cc @jondo2010 @lalten as you contributed previous versions

@fmeum
Copy link
Contributor

fmeum commented Apr 1, 2024

Bumping the compatibility level could be quite disruptive to users of the library. Could you leave an alias in place instead?

@afq984
Copy link
Contributor Author

afq984 commented Apr 1, 2024

I'm fine with aliasing

@lalten
Copy link
Contributor

lalten commented Apr 2, 2024

the diff of the BUILD is

3c3
< index 00000000..7fca1671
---
> index 00000000..f0a5da7e
6c6
< @@ -0,0 +1,127 @@
---
> @@ -0,0 +1,148 @@
73a74,94
> +)
> +
> +cc_binary(
> +    name = "zstd_cli",
> +    srcs = glob(
> +        include = [
> +            "programs/*.c",
> +            "programs/*.h",
> +        ],
> +        exclude = [
> +            "programs/datagen.c",
> +            "programs/datagen.h",
> +            "programs/platform.h",
> +            "programs/util.h",
> +        ],
> +    ),
> +    deps = [
> +        ":datagen",
> +        ":util",
> +        ":zstd",
> +    ],

It really is a shortcoming of the separate version dirs in the BCR that you can't easily see what's changed between versions :(

lalten
lalten previously approved these changes Apr 2, 2024
@lalten
Copy link
Contributor

lalten commented Apr 2, 2024

Why do you need to bump the compatibility level to add @zstd//programs:zstd if you keep @zstd//:zstd the same? One easy way would be to just patch in a separate BUILD in the programs dir

@afq984
Copy link
Contributor Author

afq984 commented Apr 2, 2024

#1730 (comment) If you mean only create the @zstd//programs:zstd in @zstd//programs and keep the rest as-is, the asymmetry of @zstd//:datagen_cli and @zstd//programs:zstd bugs me a bit. If you mean following upstream BUCK files, move everything to sub-packages and leave top level aliases, then yeah I agree it should work.

So far it seems like bumping the compatibility level causes more trouble then it's worth (maintenance, hiding @zstd//:*_sources filegroups, etc), so I'll stop pursuing bumping.

@afq984
Copy link
Contributor Author

afq984 commented Apr 4, 2024

@fmeum could you help review this PR at its current state (no structure or any compatibility level changes)? Thanks!

fmeum
fmeum previously approved these changes Apr 4, 2024
modules/zstd/metadata.json Outdated Show resolved Hide resolved
@bazel-io bazel-io dismissed stale reviews from lalten and fmeum April 4, 2024 17:41

Require module maintainers' approval for newly pushed changes.

@afq984 afq984 requested a review from fmeum April 4, 2024 19:21
@fmeum fmeum enabled auto-merge (squash) April 4, 2024 19:25
@fmeum fmeum merged commit de6cbfd into bazelbuild:main Apr 4, 2024
18 checks passed
@afq984 afq984 deleted the zstd_cli branch April 7, 2024 15:46
@afq984 afq984 mentioned this pull request Apr 7, 2024
fmeum pushed a commit that referenced this pull request Apr 7, 2024
aiuto pushed a commit to aiuto/bazel-central-registry that referenced this pull request Jun 3, 2024
aiuto pushed a commit to aiuto/bazel-central-registry that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants