esafak commented on code in PR #2357:
URL: https://github.com/apache/fory/pull/2357#discussion_r2159854203


##########
.github/workflows/release.yaml:
##########
@@ -29,7 +29,7 @@ jobs:
     strategy:
       matrix:
         python-version: [3.8, 3.9, "3.10", 3.11, 3.12, 3.13]
-        os: [ubuntu-latest, macos-13, macos-14, windows-2022]  # macos-13: 
x86, macos-14: arm64
+        os: [ubuntu-latest, ubuntu-24.04-arm, macos-13, macos-14, 
windows-2022]  # macos-13: x86, macos-14: arm64

Review Comment:
   Sure! That means I'll have to remove the x86 hard-coding in `_run_cpp`



##########
python/setup.py:
##########
@@ -43,7 +44,12 @@ class BinaryDistribution(Distribution):
     def __init__(self, attrs=None):
         super().__init__(attrs=attrs)
         if BAZEL_BUILD_EXT:
-            subprocess.check_call(["bazel", "build", "-s", "//:cp_fory_so"])
+            bazel_args = ["bazel", "build", "-s"]
+            arch = platform.machine().lower()
+            if arch in ("x86_64", "amd64"):
+                bazel_args += ["--config=x86_64"]

Review Comment:
   `x86_64` refers to the `build:x86_64` configs I added to .bazelrc
   
   They used to be added by default. I changed that in order not to invoke them 
in ARM, where they do not exist and fail the build.



##########
ci/run_ci.sh:
##########
@@ -46,41 +46,65 @@ install_pyfory() {
   popd
 }
 
+get_bazel_version() {
+    cat "$ROOT/.bazelversion"
+}
+
 install_bazel() {
   if command -v bazel >/dev/null; then
     echo "existing bazel location $(which bazel)"
     echo "existing bazel version $(bazel version)"
+    return
   fi
-  # GRPC support bazel 6.3.2 
https://grpc.github.io/grpc/core/md_doc_bazel_support.html
-  UNAME_OUT="$(uname -s)"
-  case "${UNAME_OUT}" in
-    Linux*)     MACHINE=linux;;
-    Darwin*)    MACHINE=darwin;;
-    *)          echo "unknown machine: $UNAME_OUT"
+
+  ARCH="$(uname -m)"
+  OPERATING_SYSTEM="$(uname -s)"
+
+  # Normalize architecture names
+  case "${ARCH}" in
+    x86_64|amd64)  ARCH="x86_64" ;;
+    aarch64|arm64) ARCH="arm64" ;;
+    *)             echo "Unsupported architecture: $ARCH"; exit 1 ;;
   esac
-  
URL="https://github.com/bazelbuild/bazel/releases/download/6.3.2/bazel-6.3.2-installer-$MACHINE-x86_64.sh";
-  wget -q -O install.sh $URL
-  chmod +x install.sh
-  set +x
-  ./install.sh --user
-  source ~/.bazel/bin/bazel-complete.bash
-  set -x
-  export PATH=~/bin:$PATH
-  echo "$HOME/bin/bazel version: $(~/bin/bazel version)"
-  rm -f install.sh
-  VERSION=`bazel version`
-  echo "bazel version: $VERSION"
+
+  # Handle OS-specific logic. Windows handled elsewhere
+  case "${OPERATING_SYSTEM}" in
+    Linux*)     OS="linux" ;;
+    Darwin*)    OS="darwin" ;;
+    *)          echo "Unsupported OS: $OPERATING_SYSTEM"; exit 1 ;;
+  esac
+
+  BAZEL_VERSION=$(get_bazel_version)
+  BAZEL_DIR="/usr/local/bin"
+
+  # Construct platform-specific URL
+  
BINARY_URL="https://github.com/bazelbuild/bazel/releases/download/${BAZEL_VERSION}/bazel-${BAZEL_VERSION}-${OS}-${ARCH}";
+
+  echo "Downloading bazel from: $BINARY_URL"
+  sudo wget -q -O "$BAZEL_DIR/bazel" "$BINARY_URL" || { echo "Failed to 
download bazel"; exit 1; }
+
+  sudo chmod +x "$BAZEL_DIR/bazel"
+
+  # Add to current shell's PATH
+  export PATH="$BAZEL_DIR:$PATH"
+
+  # Verify installation
+  echo "Checking bazel installation..."
+  bazel version || { echo "Bazel installation verification failed"; exit 1; }
+
+  # Configure number of jobs based on memory
   if [[ "$MACHINE" == linux ]]; then
-    MEM=`cat /proc/meminfo | grep MemTotal | awk '{print $2}'`
-    JOBS=`expr $MEM / 1024 / 1024 / 3`
-    echo "build --jobs="$JOBS >> ~/.bazelrc
+    MEM=$(grep MemTotal < /proc/meminfo | awk '{print $2}')
+    JOBS=$(( MEM / 1024 / 1024 / 3 ))
+    echo "build --jobs=$JOBS" >> /etc/bazel.bazelrc

Review Comment:
   I will restore it! It seemed to work locally.



##########
ci/deploy.sh:
##########
@@ -122,7 +122,15 @@ build_pyfory() {
   echo "Install pyfory"
   # Fix strange installed deps not found
   pip install setuptools -U
-  bazel build //:cp_fory_so
+
+  # Detect host architecture and only pass x86_64 config when appropriate
+  ARCH=$(uname -m)
+  if [[ "$ARCH" == "x86_64" || "$ARCH" == "amd64" ]]; then
+    bazel build --config=x86_64 //:cp_fory_so

Review Comment:
   How would you like to automatically apply the right build flags from 
.bazelrc? We can't leave the x86 flags enabled in ARM runs.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to