-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[Unittest][Cygwin] Set $PATH when running unittests #163947
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
Conversation
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang Author: Tomohiro Kashiwada (kikairoya) ChangesAs the Cygwin platform requires $PATH to be set in order to run unittests, do the same as for the regular Windows target. Full diff: https://github.com/llvm/llvm-project/pull/163947.diff 6 Files Affected:
diff --git a/clang-tools-extra/clangd/unittests/lit.cfg.py b/clang-tools-extra/clangd/unittests/lit.cfg.py
index 33aa9e61f4ce9..666e9879bb4ad 100644
--- a/clang-tools-extra/clangd/unittests/lit.cfg.py
+++ b/clang-tools-extra/clangd/unittests/lit.cfg.py
@@ -19,12 +19,12 @@
if platform.system() == "Darwin":
shlibpath_var = "DYLD_LIBRARY_PATH"
-elif platform.system() == "Windows":
+elif platform.system() == "Windows" or sys.platform == "cygwin":
shlibpath_var = "PATH"
else:
shlibpath_var = "LD_LIBRARY_PATH"
config.environment[shlibpath_var] = os.path.pathsep.join(
- ("@SHLIBDIR@", "@LLVM_LIBS_DIR@", config.environment.get(shlibpath_var, ""))
+ (config.shlibdir, config.llvm_libs_dir, config.environment.get(shlibpath_var, ""))
)
# It is not realistically possible to account for all options that could
diff --git a/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py b/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py
index 0963351abe3b1..c4454df06b386 100644
--- a/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py
+++ b/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py
@@ -11,12 +11,12 @@
if platform.system() == "Darwin":
shlibpath_var = "DYLD_LIBRARY_PATH"
-elif platform.system() == "Windows":
+elif platform.system() == "Windows" or sys.platform == "cygwin":
shlibpath_var = "PATH"
else:
shlibpath_var = "LD_LIBRARY_PATH"
config.environment[shlibpath_var] = os.path.pathsep.join(
- ("@SHLIBDIR@", "@LLVM_LIBS_DIR@", config.environment.get(shlibpath_var, ""))
+ (config.shlibdir, config.llvm_libs_dir, config.environment.get(shlibpath_var, ""))
)
# It is not realistically possible to account for all options that could
diff --git a/clang-tools-extra/test/Unit/lit.cfg.py b/clang-tools-extra/test/Unit/lit.cfg.py
index b7376a02c89e1..0254829ed67e4 100644
--- a/clang-tools-extra/test/Unit/lit.cfg.py
+++ b/clang-tools-extra/test/Unit/lit.cfg.py
@@ -21,7 +21,7 @@
if platform.system() == "Darwin":
shlibpath_var = "DYLD_LIBRARY_PATH"
-elif platform.system() == "Windows":
+elif platform.system() == "Windows" or sys.platform == "cygwin":
shlibpath_var = "PATH"
else:
shlibpath_var = "LD_LIBRARY_PATH"
diff --git a/clang/test/Unit/lit.cfg.py b/clang/test/Unit/lit.cfg.py
index 37e91d0f8629f..ebe35a10e7f30 100644
--- a/clang/test/Unit/lit.cfg.py
+++ b/clang/test/Unit/lit.cfg.py
@@ -51,7 +51,7 @@ def find_shlibpath_var():
yield "LD_LIBRARY_PATH"
elif platform.system() == "Darwin":
yield "DYLD_LIBRARY_PATH"
- elif platform.system() == "Windows":
+ elif platform.system() == "Windows" or sys.platform == "cygwin":
yield "PATH"
elif platform.system() == "AIX":
yield "LIBPATH"
diff --git a/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg b/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg
index 9f93bac51456d..d3eb987922995 100644
--- a/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg
+++ b/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg
@@ -2,5 +2,5 @@ import lit.formats
config.name = "googletest-cmd-wrapper"
config.test_format = lit.formats.GoogleTest(
- "DummySubDir", "Test" if "win32" in sys.platform else ".exe", [sys.executable]
+ "DummySubDir", "Test" if sys.platform in ["win32", "cygwin"] else ".exe", [sys.executable]
)
diff --git a/polly/test/Unit/lit.cfg b/polly/test/Unit/lit.cfg
index 6c450fbc54b5a..21d7bc4ab25c5 100644
--- a/polly/test/Unit/lit.cfg
+++ b/polly/test/Unit/lit.cfg
@@ -50,7 +50,7 @@ for var in [
if platform.system() == 'Darwin':
shlibpath_var = 'DYLD_LIBRARY_PATH'
-elif platform.system() == 'Windows':
+elif platform.system() == 'Windows' or sys.platform == "cygwin":
shlibpath_var = 'PATH'
else:
shlibpath_var = 'LD_LIBRARY_PATH'
|
|
@llvm/pr-subscribers-testing-tools Author: Tomohiro Kashiwada (kikairoya) ChangesAs the Cygwin platform requires $PATH to be set in order to run unittests, do the same as for the regular Windows target. Full diff: https://github.com/llvm/llvm-project/pull/163947.diff 6 Files Affected:
diff --git a/clang-tools-extra/clangd/unittests/lit.cfg.py b/clang-tools-extra/clangd/unittests/lit.cfg.py
index 33aa9e61f4ce9..666e9879bb4ad 100644
--- a/clang-tools-extra/clangd/unittests/lit.cfg.py
+++ b/clang-tools-extra/clangd/unittests/lit.cfg.py
@@ -19,12 +19,12 @@
if platform.system() == "Darwin":
shlibpath_var = "DYLD_LIBRARY_PATH"
-elif platform.system() == "Windows":
+elif platform.system() == "Windows" or sys.platform == "cygwin":
shlibpath_var = "PATH"
else:
shlibpath_var = "LD_LIBRARY_PATH"
config.environment[shlibpath_var] = os.path.pathsep.join(
- ("@SHLIBDIR@", "@LLVM_LIBS_DIR@", config.environment.get(shlibpath_var, ""))
+ (config.shlibdir, config.llvm_libs_dir, config.environment.get(shlibpath_var, ""))
)
# It is not realistically possible to account for all options that could
diff --git a/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py b/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py
index 0963351abe3b1..c4454df06b386 100644
--- a/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py
+++ b/clang-tools-extra/include-cleaner/test/Unit/lit.cfg.py
@@ -11,12 +11,12 @@
if platform.system() == "Darwin":
shlibpath_var = "DYLD_LIBRARY_PATH"
-elif platform.system() == "Windows":
+elif platform.system() == "Windows" or sys.platform == "cygwin":
shlibpath_var = "PATH"
else:
shlibpath_var = "LD_LIBRARY_PATH"
config.environment[shlibpath_var] = os.path.pathsep.join(
- ("@SHLIBDIR@", "@LLVM_LIBS_DIR@", config.environment.get(shlibpath_var, ""))
+ (config.shlibdir, config.llvm_libs_dir, config.environment.get(shlibpath_var, ""))
)
# It is not realistically possible to account for all options that could
diff --git a/clang-tools-extra/test/Unit/lit.cfg.py b/clang-tools-extra/test/Unit/lit.cfg.py
index b7376a02c89e1..0254829ed67e4 100644
--- a/clang-tools-extra/test/Unit/lit.cfg.py
+++ b/clang-tools-extra/test/Unit/lit.cfg.py
@@ -21,7 +21,7 @@
if platform.system() == "Darwin":
shlibpath_var = "DYLD_LIBRARY_PATH"
-elif platform.system() == "Windows":
+elif platform.system() == "Windows" or sys.platform == "cygwin":
shlibpath_var = "PATH"
else:
shlibpath_var = "LD_LIBRARY_PATH"
diff --git a/clang/test/Unit/lit.cfg.py b/clang/test/Unit/lit.cfg.py
index 37e91d0f8629f..ebe35a10e7f30 100644
--- a/clang/test/Unit/lit.cfg.py
+++ b/clang/test/Unit/lit.cfg.py
@@ -51,7 +51,7 @@ def find_shlibpath_var():
yield "LD_LIBRARY_PATH"
elif platform.system() == "Darwin":
yield "DYLD_LIBRARY_PATH"
- elif platform.system() == "Windows":
+ elif platform.system() == "Windows" or sys.platform == "cygwin":
yield "PATH"
elif platform.system() == "AIX":
yield "LIBPATH"
diff --git a/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg b/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg
index 9f93bac51456d..d3eb987922995 100644
--- a/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg
+++ b/llvm/utils/lit/tests/Inputs/googletest-cmd-wrapper/lit.cfg
@@ -2,5 +2,5 @@ import lit.formats
config.name = "googletest-cmd-wrapper"
config.test_format = lit.formats.GoogleTest(
- "DummySubDir", "Test" if "win32" in sys.platform else ".exe", [sys.executable]
+ "DummySubDir", "Test" if sys.platform in ["win32", "cygwin"] else ".exe", [sys.executable]
)
diff --git a/polly/test/Unit/lit.cfg b/polly/test/Unit/lit.cfg
index 6c450fbc54b5a..21d7bc4ab25c5 100644
--- a/polly/test/Unit/lit.cfg
+++ b/polly/test/Unit/lit.cfg
@@ -50,7 +50,7 @@ for var in [
if platform.system() == 'Darwin':
shlibpath_var = 'DYLD_LIBRARY_PATH'
-elif platform.system() == 'Windows':
+elif platform.system() == 'Windows' or sys.platform == "cygwin":
shlibpath_var = 'PATH'
else:
shlibpath_var = 'LD_LIBRARY_PATH'
|
As the Cygwin platform requires $PATH to be set in order to run unittests, do the same as for the regular Windows target.
7239573 to
efac315
Compare
| shlibpath_var = "LD_LIBRARY_PATH" | ||
| config.environment[shlibpath_var] = os.path.pathsep.join( | ||
| ("@SHLIBDIR@", "@LLVM_LIBS_DIR@", config.environment.get(shlibpath_var, "")) | ||
| (config.shlibdir, config.llvm_libs_dir, config.environment.get(shlibpath_var, "")) |
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.
I don't understand what this change does here and how it relates to the rest of this change, same for the file below.
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.
@SHLIBDIR@ is replaced by CMake's configuration function, so it must be in lit.site.cfg.py.in but not lit.cfg.py.
lit.cfg.py must reference variables in generated lit.site.cfg.py.
| config.llvm_libs_dir = "@LLVM_LIBS_DIR@" | |
| config.shlibdir = "@SHLIBDIR@" |
llvm-project/clang-tools-extra/clangd/unittests/CMakeLists.txt
Lines 199 to 201 in eb623e6
| configure_lit_site_cfg( | |
| ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in | |
| ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg.py) |
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 is odd, how did this code work before? Was it moved here and no one noticed that the values are no longer expanded?
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.
That sounds reasonable - but it sounds like a slightly different change from the rest of the theme of the patch. Or at the very least, it would need a mention in the commit message (PR description).
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.
Sure, I'll split out to another PR and look into how it worked before.
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 is odd, how did this code work before? Was it moved here and no one noticed that the values are no longer expanded?
For Windows, since the pre-merge CI runs checks without -DLLVM_LINK_LLVM_DYLIB=ON, an additional $PATH isn't needed. llvm-mingw appears to do the same, although its releases are configured with dylib.
For Linux, DSOs are found via RUNPATH so $LD_LIBRARY_PATH isn't needed.
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.
I'll split out to another PR
done. #164147
mstorsjo
left a comment
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.
LGTM
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/33590 Here is the relevant piece of the build log for the reference |
As the Cygwin platform requires $PATH to be set in order to run unittests, do the same as for the regular Windows target.
As the Cygwin platform requires $PATH to be set in order to run unittests, do the same as for the regular Windows target.
As the Cygwin platform requires $PATH to be set in order to run unittests, do the same as for the regular Windows target.