-
Notifications
You must be signed in to change notification settings - Fork 471
IBM Power (ppc64le) support #2091
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
base: main
Are you sure you want to change the base?
Conversation
sumitd2
commented
Aug 1, 2023
Signed-off-by: Sumit Dubey <[email protected]>
There was a problem hiding this 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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 "" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
cc: @seth-priya |