Add CMake and CI support #5604

Smjert · 2019-06-26T14:41:06Z

This PR restores CMake support, taken from osql-experimental and adapted to the use on this repository.

It gives a configuration file for Azure Pipelines, which includes a code formatting check on the Linux build.

It also fixes a build issue on Windows and macOS, related to the string_view/boost asio issue.

#5588

directionless · 2019-06-26T15:07:10Z

I'm really exited to see this!

How can I build and test it?

On my mac, I used:

rm -rf build; mkdir build && cd build
cmake ../
cmake --build . -j

And I got:

**[ 85%] Linking CXX static library libosquery_tables_events_eventstable.a
[ 85%] Built target osquery_tables_events_eventstable
[ 86%] Linking CXX static library libosquery_tables_yara_yaratable.a
[ 86%] Built target osquery_tables_yara_yaratable

Failed to retrieve the file from the given url
make[2]: *** [third-party/libmagic/libmagic-5.32.tar.gz] Error 1
make[2]: *** Deleting file `third-party/libmagic/libmagic-5.32.tar.gz'
make[1]: *** [third-party/libmagic/CMakeFiles/thirdparty_libmagic_downloader.dir/all] Error 2
make: *** [all] Error 2

Smjert · 2019-06-26T15:13:15Z

Weird, can you show me also the output of the configuration phase?
In general you need git, cmake, python@2 and python installed from brew.

I will do another PR to add status badges to the README and the documentation somewhere, but we should choose where.

On osql-experimental it was in the README, but I see that in the old master it was in the docs on readthedocs.
Also Buck build steps are kind of missing.

Smjert · 2019-06-26T15:32:34Z

Actually, I'll do another commit here for the build guide, I'll put it in the README for now.
In general though you can follow the README on https://github.com/osql/osql-experimental
A couple of caveats is that on macOS you don't need to install llvm@6 anymore and don't need to pass any -D variable to CMake when configuring.

On Windows the build tools are updated to Visual Studio 2019, they are fine, but when choosing the Desktop development with C++ Workload, on the right side also select MSVC v141.
The when configuring use
cmake ../src -G "Visual Studio 16 2019" -A Win64 -T v141,host=x64

directionless · 2019-06-26T18:43:25Z

Looks like I'm grabbing python from system, and not brew? (I don't know that this needs to be perfect to merge. I know there's other work happening on the tool chain)

Configure step:

dover:build seph$ cmake ../
-- The C compiler identification is AppleClang 10.0.1.10010046
-- The CXX compiler identification is AppleClang 10.0.1.10010046
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Not found: ccache. Install it and put it into the PATH if you want to speed up partial builds.
-- Build type: RelWithDebInfo
-- Shared libraries: OFF
-- Found Python2: /usr/local/Frameworks/Python.framework/Versions/2.7/bin/python2.7 (found version "2.7.15") found components:  Interpreter 
-- Found Python3: /usr/local/Frameworks/Python.framework/Versions/3.7/bin/python3.7 (found version "3.7.3") found components:  Interpreter 
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/seph/checkouts/osquery/osquery/build

theopolis · 2019-06-26T18:52:26Z

Reviewing now!

Smjert · 2019-06-26T19:02:28Z

Looks like I'm grabbing python from system, and not brew? (I don't know that this needs to be perfect to merge. I know there's other work happening on the tool chain)

Configure step:

dover:build seph$ cmake ../
-- The C compiler identification is AppleClang 10.0.1.10010046
-- The CXX compiler identification is AppleClang 10.0.1.10010046
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
-- Check for working C compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Not found: ccache. Install it and put it into the PATH if you want to speed up partial builds.
-- Build type: RelWithDebInfo
-- Shared libraries: OFF
-- Found Python2: /usr/local/Frameworks/Python.framework/Versions/2.7/bin/python2.7 (found version "2.7.15") found components:  Interpreter 
-- Found Python3: /usr/local/Frameworks/Python.framework/Versions/3.7/bin/python3.7 (found version "3.7.3") found components:  Interpreter 
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/seph/checkouts/osquery/osquery/build

Those are pythons from brew actually, the System one would be under /System/Library/Frameworks/...

I wonder why it does that, I just tried again locally with macOS and I previously also had a "personal" Azure Pipelines CI setup here https://dev.azure.com/smjert-test/CITest/_build/results?buildId=96 which builds fine, and it always had on osql-experimental.

I wonder if it's just AWS throwing some errors while downloading?
That part is the python script which uses urllib like this:

  try:
    print "Downloading..."
    url_opener = urllib.URLopener()
    url_opener.retrieve(url, destination_file)

  except:
    print "Failed to retrieve the file from the given url"
    return 1

Have you tried to build a second time (from scratch! so there's no doubt!)

We could eventually add more debug information when it fails

theopolis

Requesting changes to solicit your opinion. In general this looks OK. A huge plus for reducing the confusion of the previous CMake implementation.

One high-level concern I have is the sheer volume of CMake code within ./osquery. My guess is quite a bit is boilerplate. In the future can this be reduced? A good user-experience to optimize for is how much effort it takes to add a new file to the codebase. Documenting this process of "adding some code" and would be a nice follow up as well.

theopolis · 2019-06-26T19:17:39Z

CMakeLists.txt

+# This source code is licensed in accordance with the terms specified in
+# the LICENSE file found in the root directory of this source tree.
+
+cmake_minimum_required(VERSION 3.13.3)


Is this really the minimum version needed?

Yep, it's needed on Windows, to have the ability to create symlinks from CMake with its own command, it resolves an issue when VS2017 and VS2109 are installed together, it provides "target_link_options" command which is a massive improvement in how link flags are added.

In general this is a non-issue because you can download a pre-compiled binary from the CMake website, or compile it from scratch (which is not difficult at all and works pretty everywhere).

theopolis · 2019-06-26T19:17:39Z

CMakeLists.txt

+cmake_minimum_required(VERSION 3.13.3)
+project(osquery)
+
+if (BUILD_TESTING)


nitpick, inconsistent spacing if( and if used in places

👍

I'll do a pass to all the code for formatting issues.

theopolis · 2019-06-26T19:17:39Z

CMakeLists.txt

+  message(STATUS "Shared libraries: ${BUILD_SHARED_LIBS}")
+
+  if (DEFINED PLATFORM_MACOS)
+    if((NOT "${CMAKE_C_COMPILER_ID}" STREQUAL "Clang" AND NOT "${CMAKE_C_COMPILER_ID}" STREQUAL "AppleClang") OR


Does it make sense to break these out into set(APPLE_CLANG ... and LLVM_CLANG to make the conditional logic easier to read?

Well I don't think we need to have variables to actually distinguish them, maybe that can be solved just checking once for Clang but using MATCHES which searches for the word in the string.

theopolis · 2019-06-26T19:17:39Z

cmake/globals.cmake

+# This source code is licensed in accordance with the terms specified in
+# the LICENSE file found in the root directory of this source tree.
+
+cmake_minimum_required(VERSION 3.13.3)


Should we only have a single required statement in the root CMakeLists.txt?

👍
Victim of copy paste and force of habit

theopolis · 2019-06-26T19:17:39Z

cmake/globals.cmake

+elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
+  set(PLATFORM_WINDOWS 1)
+else()
+	message(FATAL_ERROR "Unrecognized platform")


nitpick, inconsistent spaces and tabs used for indent.

theopolis · 2019-06-26T19:17:39Z

cmake/wix_patches/osquery_wix_patch.xml

+        <PropertyRef Id="WIX_ACCOUNT_USERS" />
+        <PropertyRef Id="WIX_ACCOUNT_ADMINISTRATORS" />
+    </CPackWiXFragment>
+</CPackWiXPatch>


What's the issue here?

Ah I guess the spaces before />?

theopolis · 2019-06-26T19:17:39Z

osquery/CMakeLists.txt

+# This source code is licensed in accordance with the terms specified in
+# the LICENSE file found in the root directory of this source tree.
+
+cmake_minimum_required(VERSION 3.13.3)


theopolis · 2019-06-26T19:17:39Z

third-party/zlib/CMakeLists.txt

+  elseif(DEFINED PLATFORM_MACOS)
+    set(hash "7f9b44ca67eadb2c6dcf6b86688cb759b77b772858dae2a2380c032c7c1d9edd")
+  elseif(DEFINED PLATFORM_WINDOWS)
+  	set(hash "1685157a99c419e5150cc5f44a61ad1a1f5ddf414bf1a451ed9b6c7faf26d4bc")


nitpick, tab

theopolis · 2019-06-26T19:17:39Z

third-party/CMakeLists.txt

+
+# Generates a target named identifier_downloader that will acquire the remote file while also verifying
+# its hash
+function(downloadRemoteFile identifier base_url file_name hash)


I think we can remove a lot of the helper code here if we replace the dependency management in the future.

theopolis · 2019-06-26T19:17:39Z

third-party/aws-sdk-cpp/CMakeLists.txt

+
+  set(anchor_file "lib/libtesting-resources.a")
+
+  set(additional_libraries


Any thoughts about how to make this more generic?

For example you are copy-pasting the library paths save for the {a,lib}.

Yeah, in general this can be simplified even more.
If we are sure that all the .a or .lib present in the pack are needed, we could just GLOB them and use the same "importThirdPartyBinaryLibrary" to generate all the needed targets.

EDIT:
Actually I'm misremembering the issue here.
Those files are not present at configure time, so they cannot be globbed, they still don't exists.
Now I think you originally just meant to have a single list of names with code that adds .a or .lib when necessary, right?
That can be simplified but I'm not sure if it's really worth..., also on Windows sometimes the libs have different names.

Smjert · 2019-06-26T19:34:37Z

Requesting changes to solicit your opinion. In general this looks OK. A huge plus for reducing the confusion of the previous CMake implementation.

One high-level concern I have is the sheer volume of CMake code within ./osquery. My guess is quite a bit is boilerplate. In the future can this be reduced? A good user-experience to optimize for is how much effort it takes to add a new file to the codebase. Documenting this process of "adding some code" and would be a nice follow up as well.

Well boilerplate reduction is always welcome, though we have to be careful not to tailor it on the specific/current usage.
If I may, one of the pitfalls of the old CMake was a bit too much usage of global variables and custom functions that would hide where compiler flags/options could be obtained and partially reused on new targets that wouldn't necessarily need all of them etc.

Also to be fair, experimental has added a lot more libraries/targets to be built, with their own requirements, so there's more CMake code there.

Probably there are places where one could group targets which are linked over and over, we didn't do that initially because targets where still changing a lot.

I would also add that we kind of mirrored, where possible, the structure of the BUCK files.

About adding a file to a library, that is just adding a line basically.

EDIT:
Reordered a bit the comments, I've submitted it prematurely.

Smjert · 2019-06-26T22:21:59Z

[...]
A good user-experience to optimize for is how much effort it takes to add a new file to the codebase. Documenting this process of "adding some code" and would be a nice follow up as well.

To better answer this: yes! definitely, I actually had a guide in the works to explain some of the specific choices done with CMake that are needed to stay as most "compatible" with BUCK as possible.
There we can also add an how to.

Anyway I suppose that with file you meant, a new source code: that's easy.
As I said earlier, all the code was written trying to mirror BUCK, specifically so that if some new source code, dependency, or else would change in the BUCK files, it was easy to find where to change it, because it was in the CMakeLists.txt that had the same path of the BUCK file.

Comparing it to a change that doesn't come from BUCK, it's just a matter to find the CMakeLists.txt in the same folder where the change happened.
There are rare case where there's no CMakeLists.txt in the same folder (again, following BUCK files hierarchy), this just means that going up the folder hierarchy is needed.

Inside the file, it's all pretty structured and it should be easy to find where to add new source code file to be compiled with the library/executable.

Taken from osql-experimental. Initial support for Linux and macOS.

Taken from osql-experimental. - Change CMake code license to the one present in osquery right now - Package metadata doesn't mention Trail of Bits or osql anymore - Set specific ACLs for the osqueryd on Windows when packaging - Remove LLVM_INSTALL_PATH support on macOS, since we are using AppleClang - Remove OSQUERY_SOURCE_DIR variable need and source in a submodule support - Add targets format_check and format to check code formatting and format it with clang-format - Do not warn about not using Clang on macOS when using AppleClang

Only define BOOST_ASIO_DISABLE_STD_STRING_VIEW. We shouldn't define BOOST_ASIO_HAS_STD_STRING_VIEW, because even if we define BOOST_ASIO_DISABLE_STD_STRING_VIEW the first define will actually enable parts of code that will use string_view. This won't work on Windows and in general, string_view should not be used unless compiling with C++17. The hack has been also added to a test that was previously missed.

Taken from osql-experimental. - Use AppleClang compiler for macOS - Run format_check on Linux - Run pipeline only on master

Smjert · 2019-06-27T01:31:47Z

I've restored @alessandrogario initial commit for some due credits.
Sorry for the mess, I started from scratch with this at the QueryCon workshop, for simplicity, but in turn Alessandro's credits were lost.

directionless · 2019-06-27T02:11:32Z

<3 this merged

directionless added this to the 4.0.0 milestone Jun 26, 2019

theopolis requested changes Jun 26, 2019

View changes

Smjert force-pushed the Smjert:stefano/feature/cmake-support branch 2 times, most recently from 919958e to 17aa19f Jun 26, 2019

theopolis reviewed Jun 26, 2019

View changes

alessandrogario reviewed Jun 26, 2019

View changes

alessandrogario and others added 5 commits Nov 30, 2018

Initial commit

bf65cce

Taken from osql-experimental. Initial support for Linux and macOS.

Add Azure Pipelines support

cf639d4

Taken from osql-experimental. - Use AppleClang compiler for macOS - Run format_check on Linux - Run pipeline only on master

Update README and BUILD with CMake instructions

Loading status checks…

42bc51b

Smjert dismissed stale reviews from alessandrogario and theopolis via 42bc51b Jun 27, 2019

Smjert force-pushed the Smjert:stefano/feature/cmake-support branch from 17aa19f to 42bc51b Jun 27, 2019

Smjert requested review from theopolis and alessandrogario Jun 27, 2019

alessandrogario approved these changes Jun 27, 2019

View changes

Smjert merged commit ce5fee3 into osquery:master Jun 27, 2019
1 check was pending

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

Smjert deleted the Smjert:stefano/feature/cmake-support branch Oct 23, 2019

osquery / osquery

Add CMake and CI support #5604

Add CMake and CI support #5604

Smjert commented Jun 26, 2019 •

edited

directionless commented Jun 26, 2019

Smjert commented Jun 26, 2019 •

edited

Smjert commented Jun 26, 2019

directionless commented Jun 26, 2019

theopolis commented Jun 26, 2019

Smjert commented Jun 26, 2019 •

edited

theopolis left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Smjert commented Jun 26, 2019 •

edited

Smjert commented Jun 26, 2019 •

edited

Smjert commented Jun 27, 2019

directionless commented Jun 27, 2019


		set(anchor_file "lib/libtesting-resources.a")

		set(additional_libraries

osquery / osquery

Join GitHub today

Add CMake and CI support #5604

Add CMake and CI support #5604

Conversation

Smjert commented Jun 26, 2019 • edited

directionless commented Jun 26, 2019

Smjert commented Jun 26, 2019 • edited

Smjert commented Jun 26, 2019

directionless commented Jun 26, 2019

theopolis commented Jun 26, 2019

Smjert commented Jun 26, 2019 • edited

theopolis left a comment

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

Smjert Jun 26, 2019 • edited Author Contributor

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

Smjert Jun 26, 2019 Author Contributor

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

Smjert Jun 26, 2019 Author Contributor

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

Smjert Jun 26, 2019 Author Contributor

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

Smjert Jun 26, 2019 Author Contributor

This comment has been minimized.

Smjert Jun 26, 2019 Author Contributor

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

theopolis Jun 26, 2019 Member

This comment has been minimized.

Smjert Jun 26, 2019 • edited Author Contributor

Smjert commented Jun 26, 2019 • edited

Smjert commented Jun 26, 2019 • edited

Smjert commented Jun 27, 2019

directionless commented Jun 27, 2019

Essential cookies

Always active

Analytics cookies

Smjert commented Jun 26, 2019 •

edited

Smjert commented Jun 26, 2019 •

edited

Smjert commented Jun 26, 2019 •

edited

theopolis Jun 26, 2019
Member

Smjert Jun 26, 2019 •

edited

Author Contributor

theopolis Jun 26, 2019
Member

Smjert Jun 26, 2019
Author Contributor

theopolis Jun 26, 2019
Member

Smjert Jun 26, 2019
Author Contributor

theopolis Jun 26, 2019
Member

Smjert Jun 26, 2019
Author Contributor

theopolis Jun 26, 2019
Member

theopolis Jun 26, 2019
Member

Smjert Jun 26, 2019
Author Contributor

Smjert Jun 26, 2019
Author Contributor

theopolis Jun 26, 2019
Member

theopolis Jun 26, 2019
Member

theopolis Jun 26, 2019
Member

theopolis Jun 26, 2019
Member

Smjert Jun 26, 2019 •

edited

Author Contributor

Smjert commented Jun 26, 2019 •

edited

Smjert commented Jun 26, 2019 •

edited