Addressed several minor issues identified by copilot review

wspear · wspear · commit 1b6ae89067d0 · 2026-01-29T14:30:57.000-08:00
diff --git a/e4s_cl/cf/trace.py b/e4s_cl/cf/trace.py
@@ -49,12 +49,13 @@ def list_syscalls():
                 event = debugger.waitSyscall()
             except ProcessExit as event:
                 return_code = event.exitcode
-                # When a process exits (especially in MPI with multiple processes),
-                # the debugger may still have other processes. Check if we should continue.
-                if len(debugger.dict) > 0:
-                    continue
-                else:
-                    break
+                # When a process exits in a multi-process MPI run, we need to check
+                # if there are other active processes remaining. The exiting process
+                # may still be in debugger.dict at this point, so check if there are
+                # processes other than the one that just exited.
+                # Note: while debugger checks len(debugger.list), which tracks active processes.
+                # If the loop continues (debugger is still truthy), we can continue tracing.
+                continue
             except ProcessSignal as event:
                 event.process.syscall(event.signum)
                 continue
diff --git a/e4s_cl/cf/wi4mpi/__init__.py b/e4s_cl/cf/wi4mpi/__init__.py
@@ -286,6 +286,11 @@ def wi4mpi_preload(install_dir: Path, container_library_dir: str = None) -> List
     NOTE: The fake libraries (fakelib{FROM}/) go in LD_LIBRARY_PATH, NOT in LD_PRELOAD.
     This matches the official Wi4MPI launcher script behavior.
     
+    IMPORTANT: When container_library_dir is provided, this function assumes the target
+    MPI libraries (WI4MPI_RUN_MPI_C_LIB and WI4MPI_RUN_MPI_F_LIB) have already been
+    imported into the container at that location. The caller must ensure import_library()
+    is called before this function when running in container mode.
+    
     Args:
         install_dir: Path to Wi4MPI installation directory
         container_library_dir: The path where host libraries are mounted inside
diff --git a/e4s_cl/cf/wi4mpi/install.py b/e4s_cl/cf/wi4mpi/install.py
@@ -191,10 +191,13 @@ def _check_wi4mpi_install(install_dir: Path) -> bool:
                 try:
                     with open(lib, 'rb') as f:
                         elf = ELFFile(f)
-                        if elf.header.e_machine != expected:
+                        # Note: With pyelftools 0.27, header['e_machine'] returns a string like 'EM_X86_64'
+                        # Both elf.header.e_machine and elf.header['e_machine'] work equivalently
+                        machine_type = elf.header['e_machine']
+                        if machine_type != expected:
                             LOGGER.error(
                                 "Wi4MPI library %s architecture mismatch: expected %s, got %s",
-                                lib.name, expected, elf.header.e_machine)
+                                lib.name, expected, machine_type)
                             return False
                 except Exception as err:
                     LOGGER.debug("Failed to check architecture of %s: %s", lib,
diff --git a/e4s_cl/cli/commands/__execute.py b/e4s_cl/cli/commands/__execute.py
@@ -464,7 +464,8 @@ def main(self, argv):
         for path in files:
             source, dest = path, None
             if ':' in path:
-                parts = path.split(':')
+                # Split on rightmost colon to handle Unix paths that may contain colons
+                parts = path.rsplit(':', 1)
                 if len(parts) == 2:
                     source, dest = parts
                 else:
@@ -523,9 +524,8 @@ def _path(library: Library):
             # If WI4MPI is found in the environment, import its files and
             # preload its required components
             wi4mpi_import(container, wi4mpi_install_dir)
-            params.preload += wi4mpi_preload(wi4mpi_install_dir, container.import_library_dir)
             
-            # Override Wi4MPI environment variables to use container paths
+            # Import target MPI libraries BEFORE constructing preload list
             # Wi4MPI uses WI4MPI_RUN_MPI_C_LIB to dlopen the target MPI library
             # The host path won't exist inside the container; we need to use
             # the container path where host libraries are bound
@@ -544,6 +544,10 @@ def _path(library: Library):
                 if host_mpi_lib:
                     params.extra_env["WI4MPI_RUN_MPIIO_C_LIB"] = container_mpi_lib.as_posix()
                 params.extra_env["WI4MPI_RUN_MPIIO_F_LIB"] = container_mpi_f_lib.as_posix()
+            
+            # Now construct preload list AFTER libraries are imported
+            # wi4mpi_preload will use container paths that we just ensured exist
+            params.preload += wi4mpi_preload(wi4mpi_install_dir, container.import_library_dir)
 
         # Write the entry script to a file, then bind it to the container
         script_name = params.setup()
diff --git a/e4s_cl/cli/commands/profile/detect.py b/e4s_cl/cli/commands/profile/detect.py
@@ -286,9 +286,20 @@ def _filter_mpi_artifacts(libs: List[Path], files: List[Path],
     launcher = resolved_launcher.resolve()
 
     def _get_lib_prefix(path: Path) -> Path:
+        """Extract installation prefix from library path.
+        
+        Looks for 'lib' or 'lib64' directory markers to determine the prefix.
+        For example, /opt/mpich/lib/libmpi.so -> /opt/mpich
+        Falls back to parent directory for non-standard layouts.
+        """
         for marker in ['lib', 'lib64']:
             if marker in path.parts:
-                return Path(*path.parts[:path.parts.index(marker)])
+                marker_idx = path.parts.index(marker)
+                # Ensure marker is a directory component, not the filename
+                if marker_idx < len(path.parts) - 1:
+                    return Path(*path.parts[:marker_idx])
+        # Fallback: use parent directory for non-standard layouts
+        # This handles cases like /custom/path/libmpi.so
         return path.parent
 
     def _is_core_mpi(path: Path) -> bool:
diff --git a/e4s_cl/util.py b/e4s_cl/util.py
@@ -230,9 +230,15 @@ def run_e4scl_subprocess(cmd, cwd=None, env=None, capture_output=False, timeout=
             except subprocess.TimeoutExpired:
                 LOGGER.warning("Subprocess timed out after %s seconds: %s", timeout, cmd)
                 proc.kill()
-                proc.wait()
+                # Drain remaining output to avoid deadlock when process has buffered data
+                # communicate() will collect any remaining output and wait for termination
+                try:
+                    output, _ = proc.communicate(timeout=5)  # Give 5s for cleanup
+                except subprocess.TimeoutExpired:
+                    # Process still hasn't terminated after kill+communicate, force wait
+                    proc.wait()
+                    output = ""
                 returncode = -1
-                output = ""
 
     if capture_output:
         return returncode, output
diff --git a/scripts/demo.sh b/scripts/demo.sh
@@ -475,7 +475,7 @@ if [[ -n "${E4S_CL_LAUNCHER_ARGS}" ]]; then
   LAUNCHER_ARGS+=("${EXTRA_LAUNCHER_ARGS[@]}")
 fi
 
-HOST_MPI_VERSION="$(${HOST_MPIRUN} --version 2>/dev/null | head -n 2 || true)"
+HOST_MPI_VERSION="$("${HOST_MPIRUN}" --version 2>/dev/null | head -n 2 || true)"
 HOST_MPI_FAMILY="$(detect_mpi_family "${HOST_MPI_VERSION}")"
 log "Detected host MPI family: ${HOST_MPI_FAMILY:-unknown}"
 
@@ -545,10 +545,10 @@ OSU_CHECKSUM_FILE="${E4S_CL_CACHE_DIR}/osu.sha256"
 NEED_DOWNLOAD="0"
 if [[ ! -f "${OSU_TARBALL}" || ! -f "${OSU_META_FILE}" ]]; then
   NEED_DOWNLOAD="1"
-  rm -rf "${OSU_SRC_DIR}"
+  [[ -n "${OSU_SRC_DIR}" ]] && rm -rf "${OSU_SRC_DIR}"
 elif ! grep -Fq "osu_url=${E4S_CL_OSU_URL}" "${OSU_META_FILE}"; then
   NEED_DOWNLOAD="1"
-  rm -rf "${OSU_SRC_DIR}"
+  [[ -n "${OSU_SRC_DIR}" ]] && rm -rf "${OSU_SRC_DIR}"
 fi
 
 if [[ "${NEED_DOWNLOAD}" == "1" ]]; then
@@ -626,7 +626,7 @@ fi
 
 if [[ "${REBUILD_HOST_OSU}" == "1" ]]; then
   log "(Re)building host OSU benchmarks"
-  rm -rf "${OSU_HOST_PREFIX}"
+  [[ -n "${OSU_HOST_PREFIX}" ]] && rm -rf "${OSU_HOST_PREFIX}"
   mkdir -p "${OSU_HOST_PREFIX}"
   (
     # Clear CFLAGS/CXXFLAGS to avoid injecting incompatible compiler flags
@@ -707,7 +707,8 @@ EOF
     if [[ -z "${E4S_CL_APPTAINER_BUILD_ARGS}" ]]; then
       E4S_CL_APPTAINER_BUILD_ARGS="--fakeroot"
     fi
-    run_silent ${CONTAINER_CMD} build ${E4S_CL_APPTAINER_BUILD_ARGS} "${E4S_CL_IMAGE}" "${E4S_CL_IMAGE_DEF}"
+    # Note: E4S_CL_APPTAINER_BUILD_ARGS intentionally unquoted to allow word-splitting for multiple flags
+    run_silent "${CONTAINER_CMD}" build ${E4S_CL_APPTAINER_BUILD_ARGS} "${E4S_CL_IMAGE}" "${E4S_CL_IMAGE_DEF}"
   fi
 fi
 
@@ -716,9 +717,9 @@ if [[ "${E4S_CL_IMAGE}" != docker://* && "${E4S_CL_IMAGE}" != library://* ]]; th
   [[ -f "${E4S_CL_IMAGE}" ]] || fail "E4S_CL_IMAGE not found: ${E4S_CL_IMAGE}"
 fi
 
-CONTAINER_MPI_VERSION="$(${CONTAINER_CMD} exec "${E4S_CL_IMAGE}" mpirun --version 2>/dev/null | head -n 2 || true)"
+CONTAINER_MPI_VERSION="$("${CONTAINER_CMD}" exec "${E4S_CL_IMAGE}" mpirun --version 2>/dev/null | head -n 2 || true)"
 if [[ -z "$(detect_mpi_family "${CONTAINER_MPI_VERSION}")" ]]; then
-  CONTAINER_MPI_VERSION+=$'\n'"$(${CONTAINER_CMD} exec "${E4S_CL_IMAGE}" mpichversion 2>/dev/null | head -n 2 || true)"
+  CONTAINER_MPI_VERSION+=$'\n'"$("${CONTAINER_CMD}" exec "${E4S_CL_IMAGE}" mpichversion 2>/dev/null | head -n 2 || true)"
 fi
 CONTAINER_MPI_FAMILY="$(detect_mpi_family "${CONTAINER_MPI_VERSION}")"
 
@@ -774,7 +775,7 @@ run "${E4S_CL_BIN}" profile select "${E4S_CL_PROFILE_NAME}"
 # This mirrors how a user would re-run detection when the profile isn't populated.
 profile_has_bindings() {
   local output
-  output="$(${E4S_CL_BIN} profile show)"
+  output="$("${E4S_CL_BIN}" profile show)"
   if echo "${output}" | awk '/^Bound libraries:/{inlib=1; next} /^Bound files:/{inlib=0} inlib && /^ - /{print}' | grep -q .; then
     return 0
   fi
@@ -821,7 +822,7 @@ fi
 
 if [[ "${REBUILD_CONT_OSU}" == "1" ]]; then
   log "(Re)building container OSU benchmarks"
-  rm -rf "${OSU_CONT_PREFIX}"
+  [[ -n "${OSU_CONT_PREFIX}" ]] && rm -rf "${OSU_CONT_PREFIX}"
   mkdir -p "${OSU_CONT_PREFIX}"
   run_silent "${CONTAINER_CMD}" exec -B "${E4S_CL_WORKDIR}:/work" -B "${E4S_CL_CACHE_DIR}:/cache" "${E4S_CL_IMAGE}" bash -lc "\
     set -euo pipefail; \
@@ -861,9 +862,9 @@ if [[ "${E4S_CL_RUN_CONTAINER_BASELINE}" == "1" ]]; then
   for bench in "${OSU_BENCHES[@]}"; do
     bench_name="$(basename "${bench}")"
     out_file="${E4S_CL_WORKDIR}/container_${bench_name}.txt"
-    run_timed "container_${bench_name}" ${CONTAINER_CMD} exec -B "${E4S_CL_WORKDIR}:/work" "${E4S_CL_IMAGE}" bash -lc "\
-      mpirun -np ${E4S_CL_MPI_PROCS} /work/osu-container/libexec/osu-micro-benchmarks/mpi/${bench} ${OSU_ARGS[*]} \
-    " | tee "${out_file}"
+    # Build command array to avoid injection via bash -lc string interpolation
+    mpi_cmd=(mpirun -np "${E4S_CL_MPI_PROCS}" "/work/osu-container/libexec/osu-micro-benchmarks/mpi/${bench}" "${OSU_ARGS[@]}")
+    run_timed "container_${bench_name}" "${CONTAINER_CMD}" exec -B "${E4S_CL_WORKDIR}:/work" "${E4S_CL_IMAGE}" bash -lc "$(printf '%q ' "${mpi_cmd[@]}")" | tee "${out_file}"
   done
 fi
 
