Skip to content

Pass headers along as transitive dependencies #4298

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

Merged
merged 6 commits into from
May 22, 2025

Conversation

patrickmscott
Copy link
Contributor

@patrickmscott patrickmscott commented Mar 24, 2025

This fixes github.com/cloudflare/circl relative imports by making those headers available at compile time.

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Expose headers in GoArchive so that compilepkg can make them visible. This will make it possible for relative imports to find header files.

Which issues(s) does this PR fix?

Fixes #4154

Other notes for review

@patrickmscott patrickmscott marked this pull request as ready for review March 25, 2025 16:33
@albertocavalcante
Copy link
Contributor

I don't want to deviate the PR discussion, just wondering, this change alone should fix circl issues or it would also require a change in gazelle?

@patrickmscott
Copy link
Contributor Author

I don't want to deviate the PR discussion, just wondering, this change alone should fix circl issues or it would also require a change in gazelle?

In my testing, this change alone fixed circl.

@fmeum
Copy link
Member

fmeum commented May 6, 2025

@patrickmscott Friendly ping, do you have capacity to address Jay's comments?

@patrickmscott
Copy link
Contributor Author

@patrickmscott Friendly ping, do you have capacity to address Jay's comments?

Yes, it just slipped off my radar. Let me see if I can address them.

This fixes github.com/cloudflare/circl relative imports by making those
headers available at compile time.
The headers from dependencies _may_ be used to produce an artifact so
they should be direct dependencies.
- Collect headers and transitive headers into a depset.
- Provide the headers depset to emit_compilepkg
- Add a compilation target that includes headers with transitive,
  relative include paths.
Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your patience. @fmeum any last thoughts on this one?

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

No, looks good to me too!

@fmeum fmeum merged commit 393faea into bazel-contrib:master May 22, 2025
1 check passed
jvandew added a commit to foursquare/scala-gazelle that referenced this pull request Jun 2, 2025
While this repo was previously consumable as a go dependency (which is
in fact how our internal monorepo consumes it), it is also useful to
allow installation via Bzlmod or WORKSPACE for repos which otherwise
don't require a go environment. Support for these is added here,
although Bzlmod currently requires an `--override_module` flag as we
aren't published to the Bazel Central Registry.

The two main bits of complexity here are:

1. Making sure the requisite go module dependencies get installed.
Bzlmod handles this for us, but WORKSPACE requires that we set up
Gazelle to generate a macro users can call to install them.

2. Solving the patching situation with go-tree-sitter. There is an
[unreleased fix](bazel-contrib/rules_go#4298)
merged to rules_go which this PR uses to remove the need for patching in
this repo. However, we still require the patch being available for
downstream users who might not have the rules_go fix available. In the
WORKSPACE setup this is handled for them by the `scala_gazelle_deps`
macro, but Bzlmod users will have to add the `go_deps.module_override`
manually as it is only callable from their repo's root module.
@armfazh
Copy link

armfazh commented Jun 6, 2025

I've created a minimal reproducible example showing (now solved) CIRCL Bazel-Compilation.

github-merge-queue bot pushed a commit to bazel-contrib/rules_python that referenced this pull request Aug 17, 2025
)

Update rules_go to include
bazel-contrib/rules_go#4298
Update gazelle to align with the version the rules_go bzlmod will bring
in, and ensure the go.mod version is the same as bzlmod version.
Update go to 1.21 to include the `slices` library that some of the
go.mod updates depend on.

Fixes #2956.
github-merge-queue bot pushed a commit to bazel-contrib/rules_python that referenced this pull request Aug 17, 2025
)

Update rules_go to include
bazel-contrib/rules_go#4298
Update gazelle to align with the version the rules_go bzlmod will bring
in, and ensure the go.mod version is the same as bzlmod version.
Update go to 1.21 to include the `slices` library that some of the
go.mod updates depend on.

Fixes #2956.

---------

Co-authored-by: Douglas Thor <dougthor42@users.noreply.github.com>
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.

Binary that depends on cloudflare/circl can't be built cross-platform
6 participants