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

Start adapting to bzlmod configurations. #3505

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Dec 13, 2023

Some background information is at https://bazel.build/external/migration

Trying to handle the simple cases first. This adds a requirement for bazel 7 due to differences in bzlmod handling between 6 and 7 (also discussed on #infra). Bazel seems to be okay with a partial migration such as ths.

The python import behavior has subtly shifted, so carbon. is no longer part of import paths. There's a version-incompatible change for @@. bzlmod makes repos sometimes show as name~version.

target-determinator seems to be okay with @@ after a version update.

Things not moved here are things that basically need more dep work:

  • clang_register_toolchains because I need to dive into its format.
  • llvm-project because we need something slightly atypical, I need to make sure patching and the repo work carries over.
  • com_google_libprotobuf_mutator is sufficiently atypical that it doesn't have a module already, but should be one of the easier things to fix.
  • brotli/woff2: I think we should actually consider removing these. But again, they're not trivial moves.
  • treesitter due to toolchain registration, which has shifted a bit.
    • rules_nodejs because treesitter depends on it in an awkward way to migrate.

@github-actions github-actions bot added the explorer Action items related to Carbon explorer code label Dec 13, 2023
@jonmeow jonmeow force-pushed the bzlmod branch 2 times, most recently from 78f3f90 to b7aa3e1 Compare December 13, 2023 22:15
@jonmeow jonmeow marked this pull request as draft December 13, 2023 23:54
@jonmeow jonmeow removed the request for review from geoffromer December 13, 2023 23:55
@jonmeow jonmeow force-pushed the bzlmod branch 4 times, most recently from 0ad8ef8 to 1fb9a71 Compare December 14, 2023 22:15
@jonmeow jonmeow marked this pull request as ready for review December 14, 2023 22:19
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

deps_path = (
runfiles / "carbon" / "bazel" / "check_deps" / "non_test_cc_deps.txt"
)
deps_path = runfiles / "_main" / "bazel" / "check_deps" / "non_test_cc_deps.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also remove the bazel-execroot symlink, or update it to point to _main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for _main

@jonmeow jonmeow added this pull request to the merge queue Dec 15, 2023
Merged via the queue into carbon-language:trunk with commit 6204a27 Dec 15, 2023
6 checks passed
@jonmeow jonmeow deleted the bzlmod branch December 15, 2023 02:20
github-merge-queue bot pushed a commit that referenced this pull request Dec 18, 2023
Building on #3505, the toolchain and llvm rules require a little more
special-casing to get them to work well. This also moves
libprotobuffer_mutator, but that one's more minor. The migration
encounters more quirks in repo naming as seen by various queries.

This changes some of the toolchain work that was recently done for bazel
7 in #3496, dropping a bzl file I'd suggested to add, instead using
`:all` for toolchain registration. (somewhat as an improvement, somewhat
just to avoid a `load`)

Remaining in the WORKSPACE are example code repos and tree sitter rules.
Neither of these are part of the main toolchain builds, and so will
probably be lower impact if there's a good solution for them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants