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

IBM Power (ppc64le) support #2091

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

IBM Power (ppc64le) support #2091

wants to merge 23 commits into from

Conversation

sumitd2
Copy link

@sumitd2 sumitd2 commented Aug 1, 2023

...
//test/unit/utils:utils_tests_test_0                                     PASSED in 0.3s
//test/unit/utils:utils_tests_test_1                                     PASSED in 0.2s
//test/unit/utils:utils_tests_test_2                                     PASSED in 0.4s
//test/unit/utils:utils_tests_test_3                                     PASSED in 0.3s
//test/unit/utils:utils_tests_test_4                                     PASSED in 0.3s
//test/unit/versioned_libs:linux_no_version_test                         PASSED in 0.4s
//test/unit/versioned_libs:linux_suffix_version_test                     PASSED in 0.3s
//test/unit/versioned_libs:versioned_libs_unit_test_suite_test_0         PASSED in 0.3s
//test/versioned_dylib:versioned_dylib_test                              PASSED in 0.4s
//tools/runfiles:runfiles_test                                           PASSED in 0.4s
//tools/rust_analyzer:gen_rust_project_lib_test                          PASSED in 0.5s
//util/label:label_test                                                  PASSED in 0.4s
//util/process_wrapper:process_wrapper_test                              PASSED in 0.5s

Executed 321 out of 341 tests: 321 tests pass and 20 were skipped.

sumitd2 and others added 2 commits August 1, 2023 04:17
Signed-off-by: Sumit Dubey <sumit.dubey2@ibm.com>
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.

Sorry for the delay! Had a couple of questions for this one.

@@ -66,6 +73,7 @@ def crates_vendor_deps_targets():
actual = select({
":linux_amd64": "@cargo_bazel.buildifier-linux-amd64//file",
":linux_arm64": "@cargo_bazel.buildifier-linux-arm64//file",
":linux_ppc64le": "@cargo_bazel.buildifier-linux-amd64//file",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem correct. The amd64 binary can run on a powerpc CPU?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @UebelAndre Apologies for the late response, I got assigned to another project.
This one does look like a mistake. But it suggests buildifier was not required at all when I built and tested on my local ppc64le VM. Wondering where it is being used with your Intel builds?

@@ -87,6 +87,7 @@ def _cargo_build_script_impl(ctx):
flags_out = ctx.actions.declare_file(ctx.label.name + ".flags")
link_flags = ctx.actions.declare_file(ctx.label.name + ".linkflags")
link_search_paths = ctx.actions.declare_file(ctx.label.name + ".linksearchpaths") # rustc-link-search, propagated from transitive dependencies
link_search_prefix = "%s" % (ctx.label.workspace_root) if toolchain.target_arch == "powerpc64le" else ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Author

@sumitd2 sumitd2 Sep 15, 2023

Choose a reason for hiding this comment

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

@UebelAndre I had to make this change because one of the required C libraries was not getting found at the expected location. A prefix had to be added to the path but only for the power build and not the Intel build. Please allow me some time to recollect the name of the library and other specifics.

Copy link
Author

Choose a reason for hiding this comment

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

@UebelAndre The library in question is rustix_outline_powerpc64.
The path that was going in with -L was src/backend/linux_raw/arch/outline/release/
It should be external/cui__rustix-0.37.23/src/backend/linux_raw/arch/outline/release , because the library was at the location /root/.cache/bazel/_bazel_root/ff51cf56844f3e41f238392435809bdf/external/cui__rustix-0.37.23/src/backend/linux_raw/arch/outline/release/librustix_outline_powerpc64.a

I hope this makes sense.

@@ -187,7 +191,13 @@ impl BuildScriptOutput {
BuildScriptOutput::Flags(e) => compile_flags.push(e.to_owned()),
BuildScriptOutput::LinkArg(e) => compile_flags.push(format!("-Clink-arg={e}")),
BuildScriptOutput::LinkLib(e) => link_flags.push(format!("-l{e}")),
BuildScriptOutput::LinkSearch(e) => link_search_paths.push(format!("-L{e}")),
BuildScriptOutput::LinkSearch(e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this change as well?

Copy link
Author

@sumitd2 sumitd2 Sep 15, 2023

Choose a reason for hiding this comment

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

@UebelAndre I had to make this change because one of the required C libraries was not getting found at the expected location. A prefix had to be added to the path but only for the power build and not the Intel build. Please allow me some time to recollect the name of the library and other specifics.

@sumitd2
Copy link
Author

sumitd2 commented Sep 15, 2023

cc: @seth-priya

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

2 participants