[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

https://reviews.llvm.org/D38838 addresses the typo. Now that I know how it's 
supposed to work, https://reviews.llvm.org/D38839 and 
https://reviews.llvm.org/D38840 address the issues, and fix it for me.


Repository:
  rL LLVM

https://reviews.llvm.org/D38444



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny abandoned this revision.
mgorny added a comment.

Ok, I think I've found a better solution. Will start submitting patches soonish.


Repository:
  rL LLVM

https://reviews.llvm.org/D38444



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

In https://reviews.llvm.org/D38444#895403, @george.karpenkov wrote:

> @mgorny I've replied via email, but the message didn't seem to appear here.
>
> From my (maybe limited) understanding, running tests on standalone 
> compiler-rt builds was never something which was supported, as that required 
> a fresh compiler.
>  I've just tried running them, and for me even `check-*` targets don't exist.
>
> How do you create the standalone build? I've checked out `compiler-rt` 
> separately, and ran
>
>   cmake -GNinja 
> -DLLVM_CONFIG=/Users/george/code/llvm/release-build/bin/llvm-config  ../.
>


Our command-line is:

  cmake -C 
/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-_build/gentoo_common_config.cmake
 -G Ninja -DCMAKE_INSTALL_PREFIX=/usr 
-DCOMPILER_RT_INSTALL_PATH=/usr/lib/clang/6.0.0 
-DCOMPILER_RT_OUTPUT_DIR=/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-_build/lib/clang/6.0.0
 -DCOMPILER_RT_INCLUDE_TESTS=yes -DCOMPILER_RT_BUILD_BUILTINS=OFF 
-DCOMPILER_RT_BUILD_LIBFUZZER=ON -DCOMPILER_RT_BUILD_SANITIZERS=ON 
-DCOMPILER_RT_BUILD_XRAY=ON 
-DLLVM_MAIN_SRC_DIR=/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/llvm
 -DLLVM_EXTERNAL_LIT=/usr/bin/lit -DLLVM_LIT_ARGS=-vv 
-DCOMPILER_RT_TEST_COMPILER=/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-_build/lib/llvm/6/bin/clang
 
-DCOMPILER_RT_TEST_CXX_COMPILER=/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-_build/lib/llvm/6/bin/clang++
 -DCMAKE_BUILD_TYPE=RelWithDebInfo 
-DCMAKE_USER_MAKE_RULES_OVERRIDE=/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-_build/gentoo_rules.cmake
 
-DCMAKE_TOOLCHAIN_FILE=/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-_build/gentoo_toolchain.cmake
  
/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-



> but the resulting build directory does not have any `check-*` targets.

I think the `-DCOMPILER_RT_INCLUDE_TESTS=yes` may be relevant here.

> In the refactoring I did try to make the dependence conditional: all 
> compilation goes through `sanitizer_test_compile`,
>  but looking into the `sanitizer_test_compile` macro there is an obvious bug.
>  Would it be better to fix that instead?

Indeed there is. I'll check if fixing that solves the issue.


Repository:
  rL LLVM

https://reviews.llvm.org/D38444



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@mgorny I've replied via email, but the message didn't seem to appear here.

From my (maybe limited) understanding, running tests on standalone compiler-rt 
builds was never something which was supported, as that required a fresh 
compiler.
I've just tried running them, and for me even `check-*` targets don't exist.

How do you create the standalone build? I've checked out `compiler-rt` 
separately, and ran

  cmake -GNinja 
-DLLVM_CONFIG=/Users/george/code/llvm/release-build/bin/llvm-config  ../.

but the resulting build directory does not have any `check-*` targets.

In the refactoring I did try to make the dependence conditional: all 
compilation goes through `sanitizer_test_compile`,
but looking into the `sanitizer_test_compile` macro there is an obvious bug.
Would it be better to fix that instead?


Repository:
  rL LLVM

https://reviews.llvm.org/D38444



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[Michał Górny via cfe-commits]

W dniu pon, 02.10.2017 o godzinie 13∶33 -0700, użytkownik George
Karpenkov napisał:
> > On Oct 2, 2017, at 12:57 PM, Michał Górny via Phabricator 
> >  wrote:
> > 
> > mgorny added a comment.
> > 
> > In https://reviews.llvm.org/D38444#886138, @george.karpenkov wrote:
> > 
> > > > breaking stand-alone builds as a result
> > > 
> > > That's a strong statement. Could you clarify? We have a lot of buildbots 
> > > performing standalone builds, and they are still green.
> > 
> > 
> > I didn't know anyone actually added bots doing that. Are you sure we're 
> > talking about the same meaning of 'stand-alone'? Stand-alone == out of 
> > LLVM, against installed copy of LLVM.
> 
> Yes.
> You are right though that bots I was referring to do not run unit tests.
> 
> > 
> >  ninja -v -j16 -l0 check-all
> >  ninja: error: 
> > '/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-/lib/asan/tests/gtest',
> >  needed by 'lib/asan/tests/dynamic/Asan-i386-calls-Dynamic-Test', missing 
> > and no known rule to make it
> > 
> > It's as broken as it could be since it depends on target that does not 
> > exist.
> 
> Right, by “works” I’ve meant that “it compiles”, not that “unit tests pass”.
> My understanding is that running unit tests never meant to work,
> as a freshly built clang is usually needed, and in standalone mode it is not 
> available.
> I could be wrong though, in that case I do not know.

Yes, it requires some extra work to prepare a local clang copy that
works correctly but it's certainly doable.

> 
> Does everything magically work with this dummy target?

Yes, the tests work as well as they used to before the refactor (there
are a few failures but those are unrelated to the change in question).

-- 
Best regards,
Michał Górny

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[George Karpenkov via cfe-commits]

> On Oct 2, 2017, at 12:57 PM, Michał Górny via Phabricator 
>  wrote:
> 
> mgorny added a comment.
> 
> In https://reviews.llvm.org/D38444#886138, @george.karpenkov wrote:
> 
>>> breaking stand-alone builds as a result
>> 
>> That's a strong statement. Could you clarify? We have a lot of buildbots 
>> performing standalone builds, and they are still green.
> 
> 
> I didn't know anyone actually added bots doing that. Are you sure we're 
> talking about the same meaning of 'stand-alone'? Stand-alone == out of LLVM, 
> against installed copy of LLVM.

Yes.
You are right though that bots I was referring to do not run unit tests.

> 
>  ninja -v -j16 -l0 check-all
>  ninja: error: 
> '/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-/lib/asan/tests/gtest',
>  needed by 'lib/asan/tests/dynamic/Asan-i386-calls-Dynamic-Test', missing and 
> no known rule to make it
> 
> It's as broken as it could be since it depends on target that does not exist.

Right, by “works” I’ve meant that “it compiles”, not that “unit tests pass”.
My understanding is that running unit tests never meant to work,
as a freshly built clang is usually needed, and in standalone mode it is not 
available.
I could be wrong though, in that case I do not know.

Does everything magically work with this dummy target?

> 
>>> and the likeliness of people mistakenly adding more unconditional 
>>> dependencies
>> 
>> That's a good point, however I'm not sure how your change would fix the 
>> problem.
>> As far as I remember targets in compiler-rt had quite a few dependencies 
>> which required checking for whether it is a standalone build.
>> 
>> In general, I'm not sure what this change would achieve, and the added 
>> complexity can always cause more bugs in the future.
> 
> My goal is to make things work again.
> 
> 
> Repository:
>  rL LLVM
> 
> https://reviews.llvm.org/D38444
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

In https://reviews.llvm.org/D38444#886138, @george.karpenkov wrote:

> > breaking stand-alone builds as a result
>
> That's a strong statement. Could you clarify? We have a lot of buildbots 
> performing standalone builds, and they are still green.


I didn't know anyone actually added bots doing that. Are you sure we're talking 
about the same meaning of 'stand-alone'? Stand-alone == out of LLVM, against 
installed copy of LLVM.

  ninja -v -j16 -l0 check-all
  ninja: error: 
'/var/tmp/portage/sys-libs/compiler-rt-sanitizers-/work/compiler-rt-sanitizers-/lib/asan/tests/gtest',
 needed by 'lib/asan/tests/dynamic/Asan-i386-calls-Dynamic-Test', missing and 
no known rule to make it

It's as broken as it could be since it depends on target that does not exist.

>> and the likeliness of people mistakenly adding more unconditional 
>> dependencies
> 
> That's a good point, however I'm not sure how your change would fix the 
> problem.
>  As far as I remember targets in compiler-rt had quite a few dependencies 
> which required checking for whether it is a standalone build.
> 
> In general, I'm not sure what this change would achieve, and the added 
> complexity can always cause more bugs in the future.

My goal is to make things work again.


Repository:
  rL LLVM

https://reviews.llvm.org/D38444



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> breaking stand-alone builds as a result

That's a strong statement. Could you clarify? We have a lot of buildbots 
performing standalone builds, and they are still green.

> and the likeliness of people mistakenly adding more unconditional dependencies

That's a good point, however I'm not sure how your change would fix the problem.
As far as I remember targets in compiler-rt had quite a few dependencies which 
required checking for whether it is a standalone build.

In general, I'm not sure what this change would achieve, and the added 
complexity can always cause more bugs in the future.


Repository:
  rL LLVM

https://reviews.llvm.org/D38444



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38444: [compiler-rt] [cmake] Create dummy gtest target for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny created this revision.
Herald added a subscriber: dberris.

Create a dummy 'gtest' target to satisfy the test dependencies while
doing stand-alone builds. Historically those dependencies were
conditional to non-stand-alone builds but the refactoring
in https://reviews.llvm.org/rL310971 has made them unconditional, breaking 
stand-alone builds
as a result.

Given the added complexity of maintaining a large number of conditionals
scattered around the different CMakeLists.txt files and the likeliness
of people mistakenly adding more unconditional dependencies, adding
a dummy target seems an easier way forward.


Repository:
  rL LLVM

https://reviews.llvm.org/D38444

Files:
  cmake/Modules/AddCompilerRT.cmake


Index: cmake/Modules/AddCompilerRT.cmake
===
--- cmake/Modules/AddCompilerRT.cmake
+++ cmake/Modules/AddCompilerRT.cmake
@@ -291,6 +291,12 @@
   -I${COMPILER_RT_GTEST_PATH}
 )
 
+if(COMPILER_RT_STANDALONE_BUILD)
+  # Add a dummy target to satisfy dependencies on GTest when building
+  # stand-alone. GTest itself is provided via COMPILER_RT_GTEST_SOURCES.
+  add_custom_target(gtest)
+endif()
+
 append_list_if(COMPILER_RT_DEBUG -DSANITIZER_DEBUG=1 
COMPILER_RT_UNITTEST_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WCOVERED_SWITCH_DEFAULT_FLAG 
-Wno-covered-switch-default COMPILER_RT_UNITTEST_CFLAGS)
 


Index: cmake/Modules/AddCompilerRT.cmake
===
--- cmake/Modules/AddCompilerRT.cmake
+++ cmake/Modules/AddCompilerRT.cmake
@@ -291,6 +291,12 @@
   -I${COMPILER_RT_GTEST_PATH}
 )
 
+if(COMPILER_RT_STANDALONE_BUILD)
+  # Add a dummy target to satisfy dependencies on GTest when building
+  # stand-alone. GTest itself is provided via COMPILER_RT_GTEST_SOURCES.
+  add_custom_target(gtest)
+endif()
+
 append_list_if(COMPILER_RT_DEBUG -DSANITIZER_DEBUG=1 COMPILER_RT_UNITTEST_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WCOVERED_SWITCH_DEFAULT_FLAG -Wno-covered-switch-default COMPILER_RT_UNITTEST_CFLAGS)
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits