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

Prepare rust rules for Starlark CcToolchainInfo. #2424

Merged
merged 5 commits into from
Jan 16, 2024

Conversation

buildbreaker2021
Copy link
Contributor

@buildbreaker2021 buildbreaker2021 commented Jan 14, 2024

rust/private/rustc.bzl Outdated Show resolved Hide resolved
@buildbreaker2021 buildbreaker2021 changed the title Prepare rust rules for Starlark CcInfo. Prepare rust rules for Starlark CcToolchainInfo. Jan 14, 2024
@buildbreaker2021
Copy link
Contributor Author

Friendly ping

Copy link
Contributor

@daivinhtran daivinhtran left a comment

Choose a reason for hiding this comment

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

LGTM

@buildbreaker2021
Copy link
Contributor Author

@UebelAndre ptal

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

The underscore naming convention makes me nervous that we’re setting ourselves up to need to do some crazy migration and would rather not start relying on deprecated/private interfaces in the first place. This change seems fine to me but am more concerned about the conversion process for rules_cc. I also think there should be an issue made there that tracks this effort

@UebelAndre UebelAndre enabled auto-merge (squash) January 16, 2024 20:03
@UebelAndre UebelAndre merged commit 50b9ef3 into bazelbuild:main Jan 16, 2024
3 checks passed
@buildbreaker2021
Copy link
Contributor Author

The underscore naming convention makes me nervous that we’re setting ourselves up to need to do some crazy migration and would rather not start relying on deprecated/private interfaces in the first place.

About private part - API used by rules_rust was private in the first place. rules_rust is allowlisted so it can access Bazel's internal private API.

About migration part - I think the most likely scenario here is that we make that parameter public eventually. Most likely the migration which will need to be done would be to migrate from _linker_files to linker_files.

@buildbreaker2021 buildbreaker2021 deleted the patch-2 branch January 16, 2024 20:23
daivinhtran pushed a commit to daivinhtran/rules_rust that referenced this pull request Jan 17, 2024
daivinhtran pushed a commit to daivinhtran/rules_rust that referenced this pull request Jan 17, 2024
Drop import macro (bazelbuild#2411)

As discussed over at
bazelbuild#2383

Fix rustfmt toolchains when consuming rules_rust with bzlmod. (bazelbuild#2410)

Fixes bazelbuild#2260

Provide a better error message when trying to generate rust-project.json (bazelbuild#2196)

Currently when trying to generate a `rust-project.json`, if there aren't
actually any Rust targets defined, the script mysteriously fails.

This adds a better error message.

Update android example to use Starlark version of android_ndk_repository (bazelbuild#2417)

The native version of `android_ndk_repository` rule no longer work with
the newer ndk versions.

When I set `ANDROID_NDK_HOME` to
`/usr/local/vinhdaitran/Android/Sdk/ndk/26.1.10909125`, the rule expects
a different structure from the Android NDK path.
```
ERROR: /usr/local/vinhdaitran/github/rules_rust/examples/android/WORKSPACE.bazel:67:23: fetching android_ndk_repository rule //external:androidndk: java.io.IOException:
Expected directory at /usr/local/vinhdaitran/Android/Sdk/ndk/26.1.10909125/platforms but it is not a directory or it does not exist.
Unable to read the Android NDK at /usr/local/vinhdaitran/Android/Sdk/ndk/26.1.10909125, the path may be invalid. Is the path in android_ndk_repository() or ANDROID_NDK_HOME set correctly?
If the path is correct, the contents in the Android NDK directory may have been modified.
```

Using the Starlark version of the rule, as recommended in
https://bazel.build/reference/be/android#android_ndk_repository, fixes
the issue.

cc: @keith

Allow ~ in repository names (bazelbuild#2427)

Fixes bazelbuild#2426

Prepare rust rules for Starlark CcToolchainInfo. (bazelbuild#2424)

bazelbuild#2425
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

3 participants