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

deps: load cc symbols from @rules_cc #1852

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

martis42
Copy link
Contributor

@martis42 martis42 commented Apr 14, 2024

Adds dependency on rules_cc 0.0.9 for both bzlmod and workspace

This elevates rules_cc from a dev dependency to a full dependency of rules_python.

Work towards incompatible_stop_exporting_language_modules, see bazelbuild/bazel#19455

@martis42 martis42 marked this pull request as ready for review April 14, 2024 19:41
@martis42
Copy link
Contributor Author

martis42 commented Apr 14, 2024

I am a bit lost about the broken CI. I suspect that somehow in the workspace setup a too old rules_cc version is being used. Given python/repositories.bzl used maybe to fetch dependencies, this seems possible. But I am lost why only 2 workspace tests red, although there are many more using the workspace setup.

@aignas
Copy link
Collaborator

aignas commented Apr 17, 2024

Note, that the failing builds are using bazel 6.4.0. If I remember correctly, they are the only two workflows doing that. Could this be the reason?

@martis42 martis42 force-pushed the get_cc_symbols_from_rules_cc branch from 5688034 to c8c3561 Compare April 28, 2024 07:51
@martis42
Copy link
Contributor Author

@aignas
Some experimentation showed the problem is independent from the Bazel version.
I can't explain what happens exactly, but making the build green requires registering rules_cc before this line in the WORKSPACE file.
To achieve it I added rules_cc to the internal deps, which are loaded before this line. This causes rules_cc to be loaded in 2 places (internal and external deps), but given the same is already happening for bazel_skylib I assume this workaround is fine.

@rickeylev
Copy link
Collaborator

Thanks for this! It's been on my cleanup list for a long time.

Since this introduces a new dependency (it's not conceptually new, but will invoke the various remote-repo subsystems, so counts as new), please mention it in the changelog for completeness. I think rules_cc is largely just thin wrappers around the builtin-bazel rules still?

Otherwise LGTM

@rickeylev rickeylev changed the title Refactor: Fetch cc symbols from @rules_cc deps: load cc symbols from @rules_cc Apr 28, 2024
@martis42 martis42 force-pushed the get_cc_symbols_from_rules_cc branch from c8c3561 to 3e7e055 Compare April 29, 2024 16:09
Work towards incompatible_stop_exporting_language_modules, see
bazelbuild/bazel#19455

This elevates rules_cc from a dev dependency to a full dependency of rules_python.

We need to add rules_cc to the internal deps as it has to be loaded before
stardoc. Otherwise, a too old rules_cc version is used when using the legacy
WORKSPACE setup
@martis42 martis42 force-pushed the get_cc_symbols_from_rules_cc branch from 3e7e055 to 3192fb7 Compare April 29, 2024 16:11
@martis42
Copy link
Contributor Author

Yes, rules_cc are still mostly thin wrappers https://github.com/bazelbuild/rules_cc/blob/main/cc/defs.bzl#L59

@rickeylev rickeylev added this pull request to the merge queue Apr 29, 2024
Merged via the queue into bazelbuild:main with commit e3d2dad Apr 29, 2024
4 checks passed
@martis42 martis42 deleted the get_cc_symbols_from_rules_cc branch April 29, 2024 18:38
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.

3 participants