Skip to content

Conversation

@daohu527
Copy link
Contributor

add ld path in installer_base

Copilot AI review requested due to automatic review settings July 15, 2025 11:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR standardizes repository paths under the sysroot, restructures third-party Bazel rules to separate headers and shared libraries, and introduces helper logic to manage runtime library paths in installer scripts.

  • Changed new_local_repository.path to /opt/apollo/sysroot for OSQP and OpenCV, and updated their BUILD files to use cc_import + header-only cc_library.
  • Added an ensure_ld_path function in installer_base.sh and invoked it in environment setup and in the LibTorch installer to record library paths.
  • Reformatted print_usage in build_docker.sh and added a usage tip for pre-downloading packages.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
third_party/osqp/workspace.bzl Updated path to point at sysroot root instead of include subdir
third_party/osqp/osqp.BUILD Split headers and shared lib into separate cc_library & cc_import
third_party/opencv/workspace.bzl Updated path to point at sysroot root instead of include/opencv4
third_party/opencv/opencv.BUILD Refactored OpenCV targets into header-only and cc_import rules
docker/build/installers/installer_base.sh Added ensure_ld_path helper and invoked it for sysroot library
docker/build/installers/install_libtorch.sh Invoked ensure_ld_path after LibTorch install
docker/build/build_docker.sh Cleaned up print_usage formatting and added a pre-download tip
Comments suppressed due to low confidence (2)

docker/build/build_docker.sh:307

  • The previous help text included the --no-squash option and clearer labels for --clean/--no-cache; removing them could confuse users. Consider reintroducing or documenting the equivalent options explicitly.
  echo "Usage: $0 -f <Dockerfile> [options]"

docker/build/installers/install_libtorch.sh:123

  • Ensure that installer_base.sh is sourced at the top of this script so that ensure_ld_path is defined; otherwise this call will fail with a "command not found" error.
  ensure_ld_path "${INSTALL_DIR}/lib"

fi

# Add runtime library path
ensure_ld_path "${SYSROOT_DIR}/lib"
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

The return value of ensure_ld_path is not checked; if updating the ld file fails, the script will continue silently. Consider adding || exit 1 or handling errors to avoid silent misconfigurations.

Suggested change
ensure_ld_path "${SYSROOT_DIR}/lib"
if ! ensure_ld_path "${SYSROOT_DIR}/lib"; then
error "Failed to update runtime library path with ensure_ld_path."
exit 1
fi

Copilot uses AI. Check for mistakes.
@daohu527 daohu527 merged commit 7be126b into main Jul 15, 2025
1 check failed
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.

2 participants