[PATCH] D29030: [LNT] Add cross-compilation support to 'lnt runtest test-suite'

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: lnt/tests/test_suite.py:760-763
+        # FIXME: this probably needs to be conditionalized on the compiler
+        # being clang
+        if "c_target" in cmake_vars:
+            cflags += " --target=" + cmake_vars["c_target"]
----------------
MatzeB wrote:
> Shouldn't we rather put the `--target` flag into the toolchain file, is that not possible?
> 
> If we need to do it from the lnt side anyway, I'd recommend setting `TEST_SUITE_ARCH_FLAGS` which is a convention I introduced to the CMakeLists.txt. These are inserted to all compiler and linker invocations (so do not depend on our current "misfeature" that cflags are passed to C compiler, C++ compiler and linker) and plays nicer with people who override the CFLAGS in cache files.
Hi Matthias,

This _get_target_flags is used only when invoking lnt.testing.util.compilers.get_cc_info, see line 799 of this patch.
get_cc_info uses direct compiler invokes to find out information like what the target is to put into the metadata report.json.
Maybe we should let lnt.testing.util.compilers.get_cc_info also work with a cmake toolchain file, I'm not sure - but that definitly sounds something for an independent patch.
Probably the best thing to do for now is to get rid of the _get_target_flags function call and just inline the code into the one place where it's used: _get_cc_info.
So, in short, the cmake toolchain file does specify the target, but get_cc_info doesn't work with cmake toolchain files, just with command line arguments. This translates the target specified in the cmake toolchain file into what get_cc_info needs.



================
Comment at: lnt/tests/test_suite.py:766
 
-    def _get_cc_info(self):
+    def _extract_cmake_vars_from_cache(self):
         assert self.configured is True
----------------
MatzeB wrote:
> This name feels wrong to me as this does not return the cmake variables which would be `CMAKE_C_COMPILER`, `CMAKE_BUILD_TYPE`, ... but instead returns some LNT specific names like 'cc', 'build_type', ...
You're right, this should just return a dictionary with cmake variables, not LNT-specific names.


================
Comment at: lnt/tests/test_suite.py:797
+    def _get_cc_info(self):
+        cmake_vars = self._extract_cmake_vars_from_cache()
         return lnt.testing.util.compilers.get_cc_info(
----------------
MatzeB wrote:
> Looking at the earlier usage of these functions, `_extract_cmake_vars_from_cache()` is called once anyway while possible `_get_cc_info()` calls repeat this, maybe you better let the user feed in the `cmake_vars` information as an argument since it is available anyway.
_get_cc_info is actually called from 2 places, and there's a bit of plumbing cmake_vars through function calls needed.
But all in all, the required level of plumbing through variables doesn't hit my personal threshold yet for not doing so, so OK, let's do it.


https://reviews.llvm.org/D29030