C++11 and "-march=native" flags done via "modern" CMake target-based properties #27

jlblancoc · 2019-05-29T09:10:31Z

I think the title says it all :-)

The PUBLIC flags ensures that "-march=native" & "c++11" propagates to all dependent projects automatically.

dellaert · 2019-05-29T15:15:00Z

We just had an issue with "modern" cmake... Is this supported by cmake that is installed with Ubuntu 16.04?

dellaert · 2019-05-29T15:15:28Z

For record, all on board it if it works with that cmake :-)

dellaert · 2019-05-29T15:15:55Z

Modulo comment above, looks good to me, but on cmake PR's I will typically defer to @chrisbeall to do final check and merge...

jlblancoc · 2019-05-29T15:58:22Z

Ok, let's wait for @chrisbeall opinion!

We just had an issue with "modern" cmake... Is this supported by cmake that is installed with Ubuntu 16.04?

Hmm... it seems that your Jenkins instance uses Ubuntu 18.04 (CMake 3.10), and I don't have any 16.04 (with cmake 3.5) around to test. So this PR is not tested against that older version, though "modern" here normally means cmake >=3.0 so it should not be a problem, but it would be better to be sure.

Probably the best way to do this is by adding several docker images to CI?

chrisbeall · 2019-05-29T20:15:22Z

I'll test this on Ubuntu 16.04, but might not have time until Thursday evening or Friday.

jlblancoc · 2019-06-01T07:01:46Z

Rebased on top of develop so Travis con run on this PR too.

Edit: it's failing now in travis, will fix it.

dellaert · 2019-06-01T14:56:58Z

Woohoo, travis is working :-) Seems that maybe the c++11 flags do not work for all configs?

gtsam/CMakeLists.txt

dellaert · 2019-06-04T02:13:03Z

Chris, I’ll let you merge…

dellaert · 2019-06-04T17:22:55Z

@chrisbeall are you good now?

dellaert · 2019-06-04T17:23:50Z

@jlblancoc can you paste an example of new output?

jlblancoc · 2019-06-04T17:26:38Z

(I just solved the merge conflict)

Sure:

$ cmake .
-- GTSAM_SOURCE_ROOT_DIR: [/home/jlblanco/code/gtsam-jlblancoc]
-- Boost version: 1.65.1
-- Found the following Boost libraries:
--   serialization
--   system
--   filesystem
--   thread
--   program_options
--   date_time
--   timer
--   chrono
--   regex
--   atomic
-- GTSAM_BOOST_LIBRARIES: optimized;/usr/lib/x86_64-linux-gnu/libboost_serialization.so;/usr/lib/x86_64-linux-gnu/libboost_system.so;/usr/lib/x86_64-linux-gnu/libboost_filesystem.so;/usr/lib/x86_64-linux-gnu/libboost_thread.so;/usr/lib/x86_64-linux-gnu/libboost_date_time.so;/usr/lib/x86_64-linux-gnu/libboost_regex.so;debug;/usr/lib/x86_64-linux-gnu/libboost_serialization.so;/usr/lib/x86_64-linux-gnu/libboost_system.so;/usr/lib/x86_64-linux-gnu/libboost_filesystem.so;/usr/lib/x86_64-linux-gnu/libboost_thread.so;/usr/lib/x86_64-linux-gnu/libboost_date_time.so;/usr/lib/x86_64-linux-gnu/libboost_regex.so
Ignoring Boost restriction on optional lvalue assignment from rvalues
-- Could NOT find MKL (missing: MKL_INCLUDE_DIR MKL_LIBRARIES) 
-- Found Eigen version: 3.3.7
-- Building 3rdparty
-- checking for thread-local storage - found
-- Building base
-- Building geometry
-- Building inference
-- Building symbolic
-- Building discrete
-- Building linear
-- Building nonlinear
-- Building sam
-- Building sfm
-- Building slam
-- Building smart
-- Building navigation
-- GTSAM Version: 4.0.0
-- Install prefix: /usr/local
-- Building GTSAM - shared: ON
-- Building base_unstable
-- Building geometry_unstable
-- Building linear_unstable
-- Building discrete_unstable
-- Building dynamics_unstable
-- Building nonlinear_unstable
-- Building slam_unstable
-- Building partition_unstable
-- GTSAM_UNSTABLE Version: 4.0.0
-- Install prefix: /usr/local
-- Wrote /home/jlblanco/code/gtsam-jlblancoc/build/GTSAMConfig.cmake
-- ===============================================================
-- ================  Configuration Options  ======================
--   CMAKE_CXX_COMPILER_ID type     : GNU
--   CMAKE_CXX_COMPILER_VERSION     : 7.4.0
--   CMake version                  : 3.10.2
--   CMake generator                : Unix Makefiles
--   CMake build tool               : /usr/bin/make
-- Build flags                                               
--   Build Tests                    : Enabled
--   Build examples with 'make all' : Enabled
--   Build timing scripts with 'make all': Disabled
--   Build Docs                     : Enabled
--   Build shared GTSAM libraries   : Enabled
--   Put build type in library name : Enabled
--   Build libgtsam_unstable        : Enabled
--   Build for native architecture  : Enabled
--   Build type                     : Release
--   C++ compilation flags          :  -O3 -DNDEBUG
--   gtsam CXX_STANDARD             : 11
--   gtsam COMPILE_OPTIONS          : -march=native;-Wall;$<$<CONFIG:Debug>:-g;-fno-inline;-DEIGEN_INITIALIZE_MATRICES_BY_NAN>;$<$<CONFIG:RelWithDebInfo>:-g;-O3;-DNDEBUG>;$<$<OR:$<CONFIG:Release>,$<CONFIG:Profiling>>:-O3;-DNDEBUG>;$<$<CONFIG:Timing>:-g;-O3;-DNDEBUG;-DENABLE_TIMING>;-Wno-unused-local-typedefs
--   gtsam INCLUDE_DIRECTORIES      : $<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/include>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/libmetis>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/GKlib>;$<INSTALL_INTERFACE:/usr/local/include/gtsam/3rdparty/metis/>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/SuiteSparse_config>;$<INSTALL_INTERFACE:/usr/local/include/gtsam/3rdparty/SuiteSparse_config>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/CCOLAMD/Include>;$<INSTALL_INTERFACE:/usr/local/include/gtsam/3rdparty/CCOLAMD>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc>;$<INSTALL_INTERFACE:/usr/local/include/>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/build>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/CppUnitLite>;/usr/include;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/Eigen/>;$<INSTALL_INTERFACE:include/gtsam/3rdparty/Eigen/>;/usr/include
--   gtsam COMPILE_DEFINITIONS      : BOOST_OPTIONAL_ALLOW_BINDING_TO_RVALUES;BOOST_OPTIONAL_CONFIG_ALLOW_BINDING_TO_RVALUES
--   Use System Eigen               : OFF (Using version: 3.3.7)
--   Use Intel TBB                  : Yes
--   Eigen will use MKL             : MKL not found
--   Eigen will use MKL and OpenMP  : OpenMP found but GTSAM_WITH_EIGEN_MKL is disabled
--   Default allocator              : TBB
--   Build with ccache              : No
-- Packaging flags                                               
--   CPack Source Generator         : TGZ
--   CPack Generator                : TGZ
-- GTSAM flags                                               
--   Quaternions as default Rot3     : Disabled
--   Runtime consistency checking    : Disabled
--   Rot3 retract is full ExpMap     : Disabled
--   Pose3 retract is full ExpMap    : Disabled
--   Deprecated in GTSAM 4 allowed   : Enabled
--   Point3 is typedef to Vector3    : Disabled
--   Metis-based Nested Dissection   : Enabled
--   Use tangent-space preintegration: Enabled
--   Build Wrap                     : Enabled
-- MATLAB toolbox flags                                      
--   Install matlab toolbox         : Disabled
-- Cython toolbox flags                                      
--   Install Cython toolbox         : Disabled
-- ===============================================================
-- Configuring done
-- Generating done

dellaert · 2019-06-04T17:27:26Z

That's super-cool :-)

chrisbeall · 2019-06-04T17:30:19Z

I'm not a huge fan of:

--   gtsam CXX_STANDARD             : 11
--   gtsam COMPILE_OPTIONS          : -march=native;-Wall;$<$<CONFIG:Debug>:-g;-fno-inline;-DEIGEN_INITIALIZE_MATRICES_BY_NAN>;$<$<CONFIG:RelWithDebInfo>:-g;-O3;-DNDEBUG>;$<$<OR:$<CONFIG:Release>,$<CONFIG:Profiling>>:-O3;-DNDEBUG>;$<$<CONFIG:Timing>:-g;-O3;-DNDEBUG;-DENABLE_TIMING>;-Wno-unused-local-typedefs
--   gtsam INCLUDE_DIRECTORIES      : $<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/include>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/libmetis>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/GKlib>;$<INSTALL_INTERFACE:/usr/local/include/gtsam/3rdparty/metis/>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/SuiteSparse_config>;$<INSTALL_INTERFACE:/usr/local/include/gtsam/3rdparty/SuiteSparse_config>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/CCOLAMD/Include>;$<INSTALL_INTERFACE:/usr/local/include/gtsam/3rdparty/CCOLAMD>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc>;$<INSTALL_INTERFACE:/usr/local/include/>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/build>;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/CppUnitLite>;/usr/include;$<BUILD_INTERFACE:/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/Eigen/>;$<INSTALL_INTERFACE:include/gtsam/3rdparty/Eigen/>;/usr/include
--   gtsam COMPILE_DEFINITIONS      : BOOST_OPTIONAL_ALLOW_BINDING_TO_RVALUES;BOOST_OPTIONAL_CONFIG_ALLOW_BINDING_TO_RVALUES

That doesn't look very user friendly. This was easy to understand:

--   C compilation flags            :  -std=c11   -Wall -march=native -O3 -DNDEBUG
--   C++ compilation flags          :  -std=c++11 -Wall -march=native -O3 -DNDEBUG

dellaert · 2019-06-04T17:32:58Z

Two thoughts:

"gtsam " is definitely redundant on those lines, so I'd omit it.
can we have both?

chrisbeall · 2019-06-04T17:52:05Z

Can these options be shown & configured in cmake-gui and cmake-curses-gui as before? I don't see any fields there which show c++11, debug flags, etc. Should there be a field gtsam CXX_STANDARD?
New on the left, old on the right.

jlblancoc · 2019-06-04T18:13:54Z

Hmm... I guess it could be done such that the GTSAM_CMAKE_CXX_* variables are back, but then integrated into targets (gtsam, gtsam_unstable,...) via the generator expressions instead of the CMAKE_CXX_* variables.
That would be a trade-off solution, right?

There are some pros and cons in both the "old" and the "new" cmake ways, see for example:

But for library authors, I would strongly advise using the "modern" way to make exported targets as self-contained, and automatically-configured as possible. If all/most dependencies and users adhere to the "new" (cmake >=3.0) way, things are way simpler and more robust to maintain in the long term :-)

dellaert

In general, I like the changes - seems cleaner. What I am missing is maybe a comment that explains the overall scheme you propose to use. Specifically, PUBLIC vs PRIVATE and how the compile options are applied. Now is the time to add that :-)

I do see Chris's issue with communication to the users, however. Also, how can they add system specific flags? Maybe that should also be explained.

We might have to go back and forth on this a few times to make sure this improvement (which I think it is!) is also transparent and not too alien to users.

dellaert · 2019-06-05T15:23:10Z

cmake/GtsamBuildTypes.cmake

                 GTSAM_CMAKE_SHARED_LINKER_FLAGS_TIMING GTSAM_CMAKE_MODULE_LINKER_FLAGS_TIMING
-                 GTSAM_CMAKE_C_FLAGS_PROFILING GTSAM_CMAKE_CXX_FLAGS_PROFILING GTSAM_CMAKE_EXE_LINKER_FLAGS_PROFILING 
+                 GTSAM_CMAKE_C_FLAGS_PROFILING GTSAM_ GTSAM_CMAKE_EXE_LINKER_FLAGS_PROFILING


Is this a typo?

dellaert · 2019-06-05T15:23:54Z

cmake/GtsamBuildTypes.cmake

    endif()
 endif()

 if (NOT MSVC)
  option(GTSAM_BUILD_WITH_MARCH_NATIVE  "Enable/Disable building with all instructions supported by native architecture (binary may not be portable!)" ON)
  if(GTSAM_BUILD_WITH_MARCH_NATIVE)
-    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -march=native")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -march=native")
+    # Add as public flag so all dependant projects also use it, as required


dellaert · 2019-06-05T15:24:03Z

cmake/GtsamBuildTypes.cmake

-    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -march=native")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -march=native")
+    # Add as public flag so all dependant projects also use it, as required
+    # by Eigen to avid crashes due to SIMD vectorization:


dellaert · 2019-06-05T15:24:58Z

cmake/GtsamBuildTypes.cmake

+
+  # Also, ensure the compiler uses at least C++11.
+  # The use of target_compile_features() is preferred since it will be not in
+  # conflict with a more modern C++ standard, if used in a client program.


I like this!

cmake/GtsamBuildTypes.cmake

dellaert · 2019-06-05T15:26:11Z

cmake/GtsamBuildTypes.cmake

+  	target_compile_features(${target_name_} PUBLIC cxx_std_11)
+  else()
+  	if (NOT MSVC)
+  		target_compile_options(${target_name_} PUBLIC $<$<COMPILE_LANGUAGE:CXX>:-std=c++11>)


Add comment ?

dellaert · 2019-06-05T15:27:11Z

gtsam/CMakeLists.txt

-endif()
-target_compile_options(gtsam PRIVATE ${GTSAM_COMPILE_OPTIONS_PRIVATE})
+
+# Apply build flags:


in comment, say where that function is defined...

jlblancoc · 2019-06-05T22:11:40Z

PS: I'll come back to this issue soon and improve things. Thanks for the patience!

jlblancoc · 2019-06-06T22:58:29Z

Please, checkout the latest version. Options for each configuration are now editable from the cache, see attached screenshot. As CMake does, we have separate variables for preprocessor macros, and for all other compiler flags, and for "private" and "public" properties.

private: flags do only affect gtsam projects.
public: put them here only if they must be propagated to user projects (e.g. "-Wall" should not be propagated, the requirement for >=C++11 should be).

Note that the "final option variables" (those without the suffix _${build_type}) are not editable (it's mentioned in the cache "helpstring"), so users should change the ones with the per-configuration suffix.
That's why they are "marked as advanced".

Even with this limitation, it's been far from straightforward to set this up, but I think the result is much better now....
Thoughts?

PS: Still not tested on Windows.

jlblancoc · 2019-06-07T20:21:18Z

It seems there are tests failing now, will look at it...

Apparently, there are errors after not finding test datasets (?). Any wild guess of possible reasons would be appreciated... otherwise, will dig into the code. I'm not familiar (yet) with the so many tests in gtsam ;-)

dellaert · 2019-06-07T21:28:14Z

Yeah, I saw that. Maybe an option problem screwed up something in dataset.h

jlblancoc · 2019-06-08T10:05:10Z

All tests pass now. Let's see if Travis agrees...

dellaert · 2019-06-08T14:48:43Z

Segfaults? Restarted last build to confirm.

dellaert · 2019-06-09T15:11:40Z

Still segfauls :-) I recommend checking detailed build commands between develop and this branch. Seems serialization-related.

jlblancoc · 2019-06-09T21:40:54Z

The only difference I found (for both, gcc and clang) was -std=g++11 instead of -std=c++11.
Let's see if that change makes a difference...

PS: If it works and someone wants to merge, feel free of using "squash merge" to simplify all the changes into a single commit... or wait until I rebase the commits.

chrisbeall · 2019-06-09T23:19:50Z

gtsam/CMakeLists.txt

@@ -1,3 +1,5 @@
+project(gtsam LANGUAGES CXX)


Just want to make sure I understand what you did here: We now have a top-level project GTSAM, and two sub-projects gtsam and gtsam_unstable, and the main benefit of having the sub-projects is that we don't have to write /gtsam/, or is there something else less obvious that I'm missing?

Exactly, the top-level was named all upper case just to make it different from the actual library.

I normally add these project()s for many subdirectories because it's handy: for some IDEs (e.g. MSVC) CMake generates "solutions" that only include the dependent projects, so for large projects it's much faster to load only that part of the project.

Anyway, that's not why I added the two projects: it was the only way I found to prevent MSVC to fail, it was done here: 65f442c

Unfortunately, I didn't documented very well (at all, actually...) which was the error and can't remember it. Could checkout the former version just to test, will try to do it if time permits.

Bottom line anyway: something in the targets configuration broke with the new variables and the only way to make CMake to get things right was to add these project() definitions.

@chrisbeall : What do you think of the new build CMake variables? I'm afraid you will think there are too many cache entries now, but in this way things should be much easier when packaging GTSAM as a Debian packages with CMake exported projects and such, for example.

Ok, that makes sense. Definitely useful to add a comment for those kinds of workarounds in the future.
I think the new CMake variables are good, although I haven't yet tested building another project against it.
Have you had a chance to test this on Windows? Seems we always get surprises there.

jlblancoc · 2019-06-10T07:13:14Z

Hold up: I’m getting the same segfaults on another branch: https://travis-ci.com/borglab/gtsam/jobs/206627376. Might not be related to this PR at all! Only happens on Linux/clang...

wow... well, I don't know if that's good news. There is something we can try: to run valgrind at Travis.
I have that set-up for another project, see: https://github.com/MRPT/mrpt/blob/master/tests/CMakeLists.txt#L111-L125

Could be done in a separate PR to see if it detects something weird. Having make check_valgrind is useful in itself anyway.

dellaert · 2019-06-10T14:21:10Z

Not sure it's a memory issue, but adding a valgrind cmake target would be cool.
For now, I will push a travis branch with Debug on, and if that does provide a clue on how to resolve the issue, I will (atempt to) turn off linux-clang-tests and create a new issue.

Add commented out code to turn off clang on Linux.

…uted by environment variables. This should allow ccache to use caches from build stage for testing stage.

…or estimating covariance.

"const" ignored in this return type

Also: - Allow users to edit cmake target build options in the cache variables. - We had to add project() commands for gtsam and gtsam_unstable, the PROJECT_SOURCE_DIR changed, but the root GTSAM_SOURCE_DIR instead. - Ensure use of standard C++11 (no extensions)

jlblancoc · 2019-06-12T22:38:51Z

Closing, there are conflicts that have been more easily solved by cherry-picking and rebasing on a fork.
New PR is #65.

85d34351c Merge pull request #27 from borglab/fix/python3.5 dfe7639c0 support for python 3.5 git-subtree-dir: wrap git-subtree-split: 85d34351cdb29172601845f73d3281e786a531b3

chrisbeall requested changes Jun 2, 2019

View reviewed changes

gtsam/CMakeLists.txt Outdated Show resolved Hide resolved

chrisbeall reviewed Jun 2, 2019

View reviewed changes

gtsam/CMakeLists.txt Outdated Show resolved Hide resolved

jlblancoc self-assigned this Jun 3, 2019

dellaert reviewed Jun 5, 2019

View reviewed changes

jlblancoc mentioned this pull request Jun 8, 2019

Build issue with GTSAM 4.0 #43

Closed

chrisbeall reviewed Jun 9, 2019

View reviewed changes

dellaert and others added 22 commits June 13, 2019 00:28

Global env variables

646469e

Always run in debug mode

0edf123

Removed redundant flags.

bcaa61f

Add commented out code to turn off clang on Linux.

Test both Debug and Release

6d94dff

Add Debug/Release in compile stage as well

42efc13

provide

dbeeb98

add command make check_valgrind

e624d4a

targets renamed: xxx.valgrind

8977193

re-indent: script had an inconsistent mix of tabs and spaces

40250e3

added inline keywords to new functions introduced in PR #32

32e15ea

Fixed exception type for TBB path

83e4021

Use command line argument for script so that cache names are not poll…

b53d3d9

…uted by environment variables. This should allow ccache to use caches from build stage for testing stage.

exclude clang on linux

4bd45b8

Fix syntax error, add comment mentioning issue #57

e38ac41

Reduced time to run very slow testScenario by reducing sample count f…

108ee1d

…or estimating covariance.

Reduced time for ImuFactor tests

6c85332

Fixed warning

fa81e17

Turned off timing

005b30a

Unstable will not be built or tested in Debug mode: takes too long.

17642a6

fix warning

6e488db

"const" ignored in this return type

fix unused parameter warning

b538f3b

jlblancoc mentioned this pull request Jun 12, 2019

(rebased) New CMake targets-based build configuration #65

Merged

jlblancoc closed this Jun 12, 2019

jlblancoc deleted the flags_via_modern_cmake branch June 12, 2019 22:39

prashanthr05 mentioned this pull request Aug 2, 2021

BLF filters segmentation fault at getOutput call ami-iit/kindyn-vio#25

Closed

C++11 and "-march=native" flags done via "modern" CMake target-based properties #27

C++11 and "-march=native" flags done via "modern" CMake target-based properties #27

Conversation

jlblancoc commented May 29, 2019 • edited Loading

dellaert commented May 29, 2019

dellaert commented May 29, 2019

dellaert commented May 29, 2019

jlblancoc commented May 29, 2019 • edited Loading

chrisbeall commented May 29, 2019

jlblancoc commented Jun 1, 2019 • edited Loading

dellaert commented Jun 1, 2019

dellaert commented Jun 4, 2019

dellaert commented Jun 4, 2019

dellaert commented Jun 4, 2019

jlblancoc commented Jun 4, 2019

dellaert commented Jun 4, 2019

chrisbeall commented Jun 4, 2019

dellaert commented Jun 4, 2019

chrisbeall commented Jun 4, 2019

jlblancoc commented Jun 4, 2019

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlblancoc commented Jun 5, 2019

jlblancoc commented Jun 6, 2019 • edited Loading

jlblancoc commented Jun 7, 2019

dellaert commented Jun 7, 2019

jlblancoc commented Jun 8, 2019

dellaert commented Jun 8, 2019

dellaert commented Jun 9, 2019

jlblancoc commented Jun 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlblancoc commented Jun 10, 2019

dellaert commented Jun 10, 2019

jlblancoc commented Jun 12, 2019

jlblancoc commented May 29, 2019 •

edited

Loading

jlblancoc commented May 29, 2019 •

edited

Loading

jlblancoc commented Jun 1, 2019 •

edited

Loading

jlblancoc commented Jun 6, 2019 •

edited

Loading