Skip to content

Conversation

@dzhidzhoev
Copy link
Member

This commit essentially reverts https://reviews.llvm.org/D30453.

In #109961, objcopy util search code was added to dotest.py. dotest.py should use llvm-X by default if no path to a utility X is provided externally.

However, it doesn't work out for llvm-objcopy, since objcopy path is always overridden with the lines being removed here. It causes a problem with cross-platform testing when objcopy used by cmake doesn't support targets/executable file formats other than native.

I suppose these lines are unnecessary after #109961, so they can be safely removed.

This commit essentially reverts https://reviews.llvm.org/D30453.

In llvm#109961, objcopy util search code was added to dotest.py.
dotest.py should use llvm-X by default if no path to an utility X is
provided externally.

However, it doesn't work out for llvm-objcopy, since objcopy path is
always overriden with the lines being removed here. It causes a problem
with cross-platform testing when objcopy used by cmake doesn't support
targets/executable file formats other than native.

I suppose these lines are not necessary after llvm#109961, so they can be
safely removed.
@llvmbot
Copy link
Member

llvmbot commented Oct 11, 2024

@llvm/pr-subscribers-lldb

Author: Vladislav Dzhidzhoev (dzhidzhoev)

Changes

This commit essentially reverts https://reviews.llvm.org/D30453.

In #109961, objcopy util search code was added to dotest.py. dotest.py should use llvm-X by default if no path to a utility X is provided externally.

However, it doesn't work out for llvm-objcopy, since objcopy path is always overridden with the lines being removed here. It causes a problem with cross-platform testing when objcopy used by cmake doesn't support targets/executable file formats other than native.

I suppose these lines are unnecessary after #109961, so they can be safely removed.


Full diff: https://github.com/llvm/llvm-project/pull/111977.diff

1 Files Affected:

  • (modified) lldb/test/API/CMakeLists.txt (-8)
diff --git a/lldb/test/API/CMakeLists.txt b/lldb/test/API/CMakeLists.txt
index 896ce2c8c27724..2548cab5cfb5b8 100644
--- a/lldb/test/API/CMakeLists.txt
+++ b/lldb/test/API/CMakeLists.txt
@@ -96,14 +96,6 @@ if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
   endif()
 endif()
 
-if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows|Darwin")
-  list(APPEND LLDB_TEST_COMMON_ARGS_VAR
-    --env ARCHIVER=${CMAKE_AR} --env OBJCOPY=${CMAKE_OBJCOPY})
-else()
-  list(APPEND LLDB_TEST_COMMON_ARGS_VAR
-    --env OBJCOPY=${LLVM_TOOLS_BINARY_DIR}/llvm-objcopy${CMAKE_EXECUTABLE_SUFFIX})
-endif()
-
 if (NOT "${LLDB_LIT_TOOLS_DIR}" STREQUAL "")
   if (NOT EXISTS "${LLDB_LIT_TOOLS_DIR}")
     message(WARNING "LLDB_LIT_TOOLS_DIR ${LLDB_LIT_TOOLS_DIR} does not exist.")

Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

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

LGTM. LLDB's build/test setup is a lot different that what it was when I wrote that patch.

@vvereschaka
Copy link
Contributor

@dzhidzhoev is sick. I'll merge this MR for him.

@vvereschaka vvereschaka merged commit 1cb8314 into llvm:main Oct 15, 2024
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
…lvm#111977)

This commit essentially reverts https://reviews.llvm.org/D30453.

In llvm#109961, objcopy util search code was added to dotest.py. dotest.py
should use llvm-X by default if no path to a utility X is provided
externally.

However, it doesn't work out for llvm-objcopy, since objcopy path is
always overridden with the lines being removed here. It causes a problem
with cross-platform testing when objcopy used by cmake doesn't support
targets/executable file formats other than native.

I suppose these lines are unnecessary after llvm#109961, so they can be
safely removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants