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

Add circl 1.3.3 #674

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Add circl 1.3.3 #674

merged 2 commits into from
Jun 2, 2023

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented May 31, 2023

This is a Go dependency that requires custom patches beyond what Gazelle can generate automatically due to its use of CGo with headers referenced across packages.

@fmeum fmeum added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label May 31, 2023
This is a Go dependency that requires custom patches beyond what Gazelle
can generate automatically due to its use of CGo with headers referenced
across packages.
@fmeum fmeum marked this pull request as ready for review May 31, 2023 19:50
@fmeum
Copy link
Contributor Author

fmeum commented May 31, 2023

@meteorcloudy Would you consider this patch size to be acceptable in the long term? It's going to be similar or higher for other Go dependencies.

@meteorcloudy
Copy link
Member

Is there any chance to upstream the BUILD file? Or have any other way of maintaining it? For example, have it in a fork and some maintainers can review any BUILD file change.

@fmeum
Copy link
Contributor Author

fmeum commented Jun 2, 2023

I could ask the upstream maintainers, but since this project is not using Bazel and the BUILD files can easily go out of sync with the state of the repo, I doubt that they will feel good about maintaining them.

A fork would be possible, but have downsides in terms of supply chain security: With the current approach, it is very easy for users to verify that they are getting just the actual module + BUILD files. A fork would obscure this information.

Given the scale of repos such as https://github.com/DefinitelyTyped/DefinitelyTyped, maybe having this amount of patches in the BCR is not a big problem? I just wanted to bring up this discussion before committing to this approach in rules_go.

CC @linzhp @tyler-french in case you have ideas

@meteorcloudy
Copy link
Member

meteorcloudy commented Jun 2, 2023

In general, I'm fine with a patch for BUILD files at this size, but a bit worried about the BUILD file content isn't actually reviewed by someone else who is familiar with the project.

@fmeum
Copy link
Contributor Author

fmeum commented Jun 2, 2023

Most of the patch is what Gazelle generates automatically. I will try to split up the patch to make this process more transparent and easier to update on new releases.

@fmeum
Copy link
Contributor Author

fmeum commented Jun 2, 2023

@meteorcloudy I split up the patches and added commit messages, including the command used to generate BUILD files. Do you think this is now sufficiently transparent without involving upstream?

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@meteorcloudy meteorcloudy merged commit 067eb50 into bazelbuild:main Jun 2, 2023
@fmeum fmeum deleted the circl-1.3.3 branch June 2, 2023 12:41
alexeagle pushed a commit to aspect-build/bazel-central-registry that referenced this pull request Jun 26, 2023
* Add circl 1.3.3

This is a Go dependency that requires custom patches beyond what Gazelle
can generate automatically due to its use of CGo with headers referenced
across packages.

* Split up patch into generated and hand-crafted parts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants