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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks, LGTM still stands.



================
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"]
----------------
kristof.beyls wrote:
> 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.
> 
Ah ok, I missed that it is not self.cflags here, but something used locally. That's certainly ok for now.

Long term I agree that we should rather have cmake fill out the cc information and place that in a file or similar, but that is for another patch.


================
Comment at: lnt/tests/test_suite.py:761-774
+                "CMAKE_C_COMPILER",
+                "CMAKE_BUILD_TYPE",
+                "CMAKE_CXX_FLAGS",
+                "CMAKE_CXX_FLAGS_DEBUG",
+                "CMAKE_CXX_FLAGS_MINSIZEREL",
+                "CMAKE_CXX_FLAGS_RELEASE",
+                "CMAKE_CXX_FLAGS_RELWITHDEBINFO",
----------------
Maybe we should just extract them all instead of maintaining a whitelist. But I don't really care either way.


https://reviews.llvm.org/D29030