Knutel/peter d cmake include fixes #397

ghost · 2023-02-06T17:02:24Z

No description provided.

peter-d

Much cleaner already, nice - but a few remarks in the comments.

Also I was thinking: it's not very easy to see based on the config file what I need to do if I build a project to also link against libsparta or libsimdb...

It would be the most user friendly if you can just add Sparta to the target_link_libraries statement.

sparta/cmake/sparta-config.cmake

sparta/CMakeLists.txt

peter-d · 2023-02-06T19:14:28Z

And we did still break the examples in CI, though they build in this branch for me locally...

ghost · 2023-02-06T19:34:13Z

It would be the most user friendly if you can just add Sparta to the target_link_libraries statement.

Agreed!

peter-d · 2023-02-08T15:27:21Z

Hi Knute,

In trying to get my cmake file to find sparta up and running and testing it on building Olympia, I've noticed that Olympia depends on a few parts of the sparta tests folder:

TestingMacros.cmake which has convenience macros
but also a bunch of (test?) C++ code from sparta/tests

I was currently moving to a setup where make install installs:

include files
libsparta.a and libsimdb.a
and also the cmake files that other projects need to link against sparta

Should the test code be installed as well? Right now make install does not include these. Or is Olympia an exceptional case?

That said: I've come to a point where it can Olympia can find sparta after a make install, generate all makefiles but then fails as it cannot include files from map/sparta/test (in my case I need to point it to the right folder with a command line arg, as it is not installed in a system wide location like /usr/local or such)

ghost · 2023-02-08T17:01:18Z

TestingMacros.cmake which has convenience macros

Yep, expected.

but also a bunch of (test?) C++ code from sparta/tests

Olympia depends on the tests? Odd (and sounds broken).

I was currently moving to a setup where make install installs:

include files

libsparta.a and libsimdb.a

and also the cmake files that other projects need to link against sparta

Should the test code be installed as well? Right now make install does not include these. Or is Olympia an exceptional case?

My original intent was just a dependency on the TestingMacros.cmake. I'm wondering if the install target could copy that file over... But your previous statement has be concerned.

That said: I've come to a point where it can Olympia can find sparta after a make install, generate all makefiles but then fails as it cannot include files from map/sparta/test (in my case I need to point it to the right folder with a command line arg, as it is not installed in a system wide location like /usr/local or such)

That's AWESOME! And a bummer on the testing. None of this is set in stone. If this becomes too complex, other routes can be explored.

Thanks for helping with this.

peter-d · 2023-02-08T18:54:19Z

My original intent was just a dependency on the TestingMacros.cmake. I'm wondering if the install target could copy that file over... But your previous statement has be concerned.

That’s what I did now. It sits next to FindSparta.cmake and gets included when you find_package(Sparta). But turns out that’s not enough to get Olympia to build.

I’m not at my computer tonight but will do some more digging on the test dependencies once I can.

peter-d · 2023-02-09T07:45:14Z

The culprit is Dispatch, which instantiates a WeightedContextCounter which is included from sparta source tree: test/ContextCounter/WeightedContextCounter.hpp: https://github.com/riscv-software-src/riscv-perf-model/blob/697fdc2a950be59e67a78a53220f3eb2a0c03e39/core/Dispatch.hpp#L19

A quick grep shows that this is the only place where this happens.

Which then of course gives problems:

[ 41%] Building CXX object core/CMakeFiles/core.dir/Dispatch.cpp.o
cd /Users/pdback/src/peter-d/riscv-perf-model/release/core && /Library/Developer/CommandLineTools/usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_CHRONO_DYN_LINK -DBOOST_FILESYSTEM_DYN_LINK -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DBOOST_SERIALIZATION_DYN_LINK -DBOOST_TIMER_DYN_LINK -DOLYMPIA_VERSION=\"v0.1.0\" -I/Users/pdback/src/peter-d/riscv-perf-model/core -I/Users/pdback/src/peter-d/riscv-perf-model/mss -isystem /opt/homebrew/include -isystem /Users/pdback/src/peter-d/riscv-perf-model/mavis -isystem /Users/pdback/src/peter-d/riscv-perf-model/stf_lib -isystem /simdb/include -isystem /opt/homebrew/Cellar/[email protected]/1.76.0_3/include -isystem /Users/pdback/opt/sparta/include -isystem /opt/homebrew/Cellar/hdf5/1.12.2_2/include -isystem /opt/homebrew/opt/libaec/include -O3 -DNDEBUG -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk -Wall -Wextra -Winline -Winit-self -Wno-unused-function -Wuninitialized -Wno-sequence-point -Wno-inline -Wno-unknown-pragmas -Woverloaded-virtual -Wno-unused-parameter -Wno-missing-field-initializers -pipe -O3 -mllvm -inline-threshold=1024 -fomit-frame-pointer -flto=thin -std=c++17 -fPIC -Wconversion -std=gnu++17 -MD -MT core/CMakeFiles/core.dir/Dispatch.cpp.o -MF CMakeFiles/core.dir/Dispatch.cpp.o.d -o CMakeFiles/core.dir/Dispatch.cpp.o -c /Users/pdback/src/peter-d/riscv-perf-model/core/Dispatch.cpp
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/Dispatch.cpp:4:
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/CoreUtils.hpp:10:
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/CPUTopology.hpp:9:
In file included from /Users/pdback/src/peter-d/riscv-perf-model/core/CPUFactories.hpp:11:
/Users/pdback/src/peter-d/riscv-perf-model/core/Dispatch.hpp:19:10: fatal error: 'test/ContextCounter/WeightedContextCounter.hpp' file not found
#include "test/ContextCounter/WeightedContextCounter.hpp"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [core/CMakeFiles/core.dir/Dispatch.cpp.o] Error 1
make[1]: *** [core/CMakeFiles/core.dir/all] Error 2
make: *** [all] Error 2

Looks to me like either that example weighted counter is useful enough to be part of sparta proper (there might be others like it?), or we should add the file to Olympia as a utility defined and used locally there.

peter-d · 2023-02-09T07:53:29Z

I've pushed what I have for now - FindSparta.cmake courtesy of my colleague @tomhaber, who put this together.

A few things we should fix here before this is ready to merge:

I'd like to use the find_package construct for the tests or at least a test, so it's covered in regression and we can see it break there rather than in projects depending on sparta.
This is now a manually generated cmake file, but the more modern, proper way is to use install(EXPORT ...) which will likely be more robust
We should add/update something in the README or docs on how to use sparta in other projects.

peter-d · 2023-02-09T09:24:23Z

So, ignoring the dependency on the counter from test, I also see Olympia needs map/sparta/cache code, which is also not installed in the default make install, which only copies over map/sparta/sparta to the installation destination.

What's the intent there? is cache supposed to be a fixed part of sparta? Or is it more like stf_lib and mavis, a set of utilities you can optionally use on top of/with sparta?

Depending on that answer, it's relatively easy to install it, but question is if we want to/should.

…ackage(PythonInterp)

peter-d · 2023-02-09T13:30:47Z

I had a quick look at pipeViewer/argos as well on how to install it properly, so it can be nicely used from projects like Olympia without needing the full sparta source as dependency, but it's net very clear what depends on what, with the mix of CPP, Python and Cython and python setup.py to build parts of it.

If you know how that's structured, I can help pulling out all the right bits at make install so people can just put an Argos script somewhere in their path to use it.

peter-d · 2023-02-09T13:54:05Z

3af0543 fails in CI :(

On my system (no conda), cmake would find the right python (from homebrew) with find_package(PythonInterp), but then in the actual setup.py it would just use whatever cmake thinks is there. (which does not have wx, cython and other dependencies installed)

So I fixed that, such that it finds the PythonInterp (and I can overrule that one from cmd line if needed, with the standard options). But that seems to now find the wrong one in CI... (probably due to the note here)

I've updated to the recommended more modern way to find Python3, let's hope that fixes CI...

ps this is quickly becoming a very deep rabbit hole ;-)

ghost · 2023-02-09T22:34:51Z

Looks to me like either that example weighted counter is useful enough to be part of sparta proper

Ugh, I have NO idea how that happened. Yes, it can be made proper esp since it has it own test. I'll work on that.

ghost · 2023-03-01T03:21:54Z

Getting closer! The ArgosDumper tool needs the rt library in Linux, but it's not part of the SPARTA::sparta library list. Also, simdb isn't found for the transactiondb.so link.

ghost · 2023-03-03T00:21:07Z

Yahoo!!! 100% pass!

peter-d · 2023-03-03T08:23:30Z

Great! I can confirm it still builds nicely on my end as well. One minor nitpick, I get (got it before as well I think, so it's not a side effect from the last few commits) a warning from setuptools on pyrex_gdb=True, and I see it's there in CI as well:
`UserWarning: Unknown Extension options: 'pyrex_gdb'

Other than that: looks good to go for me!
Thank you for dotting and crossing the final I'd and T's!

…e_include_fixes

… into knutel/peter-d_cmake_include_fixes

ghost · 2023-03-13T17:32:40Z

Gonna shoot for merging this PR this week.

peter-d · 2023-03-13T18:14:11Z

Anything I can/should do to help?

ghost · 2023-03-13T18:16:20Z

Nope! About the push the Big Green Button! We needed to shut down auto-pulls and testing with our models here.

ghost · 2023-03-13T18:38:31Z

Actually, Peter, do you mind putting up your PR for the risc-v performance model again?

peter-d · 2023-03-13T18:50:14Z

It’s there: riscv-software-src/riscv-perf-model#42

Overview -------- Overhaul of MAP build system and included proper support for installation Updated Documentation Updated/Cleaned up Argos and simplified build system Various bug fixes Details ------- * simpler boost detection on mac * cmake clean up: target specific include paths * boost 1.76 * boost 1.72 specific magic no longer needed * Moved reliance on include paths back to sparta.cmake file * Made simdb include a private include * PUBLIC includes for Sparta and SimDB to pass to regression * cmake file to enable find_package(Sparta) in other projects * make sure core setup.py actually uses the interpreter found by find_package(PythonInterp) * more modern way to find python3, hopefully also robust in conda/venv * cleaner cmake for testing macros for simdb and sparta * helios build now using FindSparta.cmake * pipeviewer cannot use find_package(sparta), which only works once installed * installing all py files and libs but module paths don't work yet after install * simpler setup for cython build - working for transactiondb, rest todo * cleaner HDF5 libs config for sparta * Documentation cleanup * Major overhaul of how argos ties into cmake * cmake pushes all config into setuptools via setup.cfg where possible (such that all found libs in cmake and sparta deps are automatically in sync with rest of the build) * python setup.py then manages the cythonized build *and* installation of all python files by doing python setup.py install * Cleaned up all python imports so they are relative imports (previously mixed absolute vs relative which breaks when you install the package) * relocated some of the cython files in the source tree for easier build and install * Fixed static method calls w/o object * Move __is_mac_os and __gen_message outside of the ShortcutHelp class * Clean up flake8 errors * make sure setuptools and cython use the right C/CXX/LD * proper dependencies for setup.py * gui script vs console script: should properly call pythonw * build argos_dumper again * _ vs __ otherwise cannot find the free helper functions in the class * Move sparta library target towards the end of sparta/CMakeLists.txt so it picks up all of the compile/link flags Bump required Boost version down to 1.74 so we can build on default Ubuntu environment * Pass CMAKE_INSTALL_PREFIX to setup.py install - Enables installing package to a non-default directory * Add Github workflow to run linters on Argos Reorganize mypy stubs and .ini locations to work with relative imports in Argos Use pip instead of (deprecated) setup.py install to install Argos - Also properly handle non-default install prefix * Set a a CMAKE_MACRO file path for the testers; allows sparta build anywhere * Update README.md for Conda on MacOS * seperated build for sparta and helios folder * Updated README and build rules for new layout * Fixed bad dir path in regression * Renamed DatabaseDump tool * Added missing rt library for Linux builds * Put in check for CPP check module * Fixed new compilation issue with HDF test * ANOTHER HDF5 test fix * Forced HDF5 test to be C++ testing * Removed rt library link * ONE more tweak: use CXX version of HDF5 * Fixed formatting * Updated README with install directions --------- Co-authored-by: Peter Debacker <[email protected]> Co-authored-by: Brett Dutro <[email protected]> Co-authored-by: Benoy Alexander <[email protected]> 4033e18

This fixes #35 and tracks the changes in Sparta [PR397(sparcians/map#397), which are merges into Sparta main already. --------- Signed-off-by: Peter Debacker <[email protected]> Signed-off-by: Knute Lingaard <[email protected]> Co-authored-by: Peter Debacker <[email protected]> Co-authored-by: Knute Lingaard <[email protected]> Co-authored-by: Knute Lingaard <[email protected]>

timsnyder · 2024-03-11T21:46:27Z

sparta/simdb/test/CoreDatabase/CMakeLists.txt

@@ -1,8 +1,7 @@
 project(SIMDB_CoreDatabase_test)

 add_executable(SIMDB_CoreDatabase_test CoreDatabase_test.cpp)
-
-include(../TestingMacros.cmake)
+include(${SPARTA_CMAKE_MACRO_PATH}/SimdbTestingMacros.cmake)


Overall, this PR fixes quite a few things but I'm coming across this. Putting SimdbTestingMacros.cmake in Sparta's cmake/ subdirectory and defining SPARTA_CMAKE_MACRO_PATH in the toplevel sparta/CmakeLists.txt makes it so that simdb can't be built and regressed on it's own as a "cmake project". @klingaard Was this intended? Seems slightly odd if not broken to me.

Good catch - I think this is indeed odd to say the least. It probably should be in sparta/simdb/cmake.
It should probably also be removed here as well:

map/sparta/cmake/FindSparta.cmake

Line 90 in 64e119f

include(${CMAKE_CURRENT_LIST_DIR}/SimdbTestingMacros.cmake)

For context, as far as I remember correctly:

SpartaTestingMacros.cmake needs to be exposed with all the other cmake files for sparta, wherever sparta is built or installed, as olympia and possibly other projects depend on it: it uses the sparta_regress macro. That is why I moved that file to the toplevel cmake folder. (that's not a very clean approach in olympia, imo, but we didn't want to break that)

I think @klingaard and I did the same for SimdbTestingMacros.cmake, simply by analogy. But it's probably not needed outside the sparta build tree. And even if it were, we should do that in such a way that simdb can be built and run stand-alone.

As a side note: SimdbTestingMacros.cmake is now included in every CMakeLists.txt file in the subfolder for every simdb test. Won't it be more logical (and cleaner) to include this once, 1 level up in sparta/simdb/test/CMakeLists.txt?

Peter Debacker and others added 7 commits February 3, 2023 15:13

simpler boost detection on mac

098160b

cmake clean up: target specific include paths

b2b5a2e

boost 1.76

e141720

boost 1.72 specific magic no longer needed

adce649

Merge remote-tracking branch 'origin/master' into HEAD

296f7ef

Merged Peter's master branch with some cmake fixes on my end

93a031f

Moved reliance on include paths back to sparta.cmake file

120aafd

ghost self-assigned this Feb 6, 2023

ghost mentioned this pull request Feb 6, 2023

simpler boost detection on mac #396

Closed

Made simdb include a private include

39c889e

peter-d reviewed Feb 6, 2023

View reviewed changes

sparta/cmake/sparta-config.cmake Show resolved Hide resolved

sparta/CMakeLists.txt Outdated Show resolved Hide resolved

Knute Lingaard added 2 commits February 6, 2023 14:16

PUBLIC includes for Sparta and SimDB to pass to regression

1f7ed09

Small cleanup; added missing SimDB testers target include

cc8f62f

cmake file to enable find_package(Sparta) in other projects

667ec0c

Peter Debacker added 3 commits February 9, 2023 11:32

make sure we include the testing macros

4c3ab3f

also installing sparta/cache

736df39

make sure core setup.py actually uses the interpreter found by find_p…

3af0543

…ackage(PythonInterp)

more modern way to find python3, hopefully also robust in conda/venv

eac0f29

ghost mentioned this pull request Feb 9, 2023

Tell cmake to add boost includes for core and mss riscv-software-src/riscv-perf-model#36

Closed

Knute Lingaard added 4 commits March 2, 2023 09:58

Added missing rt library for Linux builds

cd34a5d

Put in check for CPP check module

3482904

Fixed new compilation issue with HDF test

3b76800

ANOTHER HDF5 test fix

29992e5

ghost mentioned this pull request Mar 2, 2023

test buffer works like std::list #410

Merged

Knute Lingaard added 2 commits March 2, 2023 14:23

Forced HDF5 test to be C++ testing

2ebd540

Removed rt library link

39cc0ed

ONE more tweak: use CXX version of HDF5

8a4c272

ghost added enhancement Enhancement or request component: sparta Issue is related to sparta framework component: plato issues related to Plato (part of Helios) labels Mar 5, 2023

Knute Lingaard and others added 4 commits March 5, 2023 19:27

Merge remote-tracking branch 'origin/master' into knutel/peter-d_cmak…

fc9ec7f

…e_include_fixes

Merge remote-tracking branch 'origin/benoy-sifive-readme_conda_macos'…

53f223f

… into knutel/peter-d_cmake_include_fixes

Fixed formatting

895c7b1

Updated README with install directions

22dcecc

peter-d mentioned this pull request Mar 13, 2023

Update boost deps for core and mss, track Sparta PR397 riscv-software-src/riscv-perf-model#42

Merged

ghost merged commit 4033e18 into master Mar 13, 2023

ghost deleted the knutel/peter-d_cmake_include_fixes branch March 13, 2023 20:51

peter-d mentioned this pull request Apr 28, 2023

Boost cmake: why use Boost_NO_BOOST_CMAKE ON on macOs? #395

Closed

timsnyder reviewed Mar 11, 2024

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Knutel/peter d cmake include fixes #397

Knutel/peter d cmake include fixes #397

ghost commented Feb 6, 2023

peter-d left a comment

peter-d commented Feb 6, 2023

ghost commented Feb 6, 2023

peter-d commented Feb 8, 2023

ghost commented Feb 8, 2023 •

edited by ghost

Loading

peter-d commented Feb 8, 2023

peter-d commented Feb 9, 2023

peter-d commented Feb 9, 2023 •

edited

Loading

peter-d commented Feb 9, 2023

peter-d commented Feb 9, 2023

peter-d commented Feb 9, 2023

ghost commented Feb 9, 2023

ghost commented Mar 1, 2023

ghost commented Mar 3, 2023

peter-d commented Mar 3, 2023

ghost commented Mar 13, 2023

peter-d commented Mar 13, 2023

ghost commented Mar 13, 2023

ghost commented Mar 13, 2023

peter-d commented Mar 13, 2023

timsnyder Mar 11, 2024

peter-d Mar 12, 2024

Knutel/peter d cmake include fixes #397

Knutel/peter d cmake include fixes #397

Conversation

ghost commented Feb 6, 2023

peter-d left a comment

Choose a reason for hiding this comment

peter-d commented Feb 6, 2023

ghost commented Feb 6, 2023

peter-d commented Feb 8, 2023

ghost commented Feb 8, 2023 • edited by ghost Loading

peter-d commented Feb 8, 2023

peter-d commented Feb 9, 2023

peter-d commented Feb 9, 2023 • edited Loading

peter-d commented Feb 9, 2023

peter-d commented Feb 9, 2023

peter-d commented Feb 9, 2023

ghost commented Feb 9, 2023

ghost commented Mar 1, 2023

ghost commented Mar 3, 2023

peter-d commented Mar 3, 2023

ghost commented Mar 13, 2023

peter-d commented Mar 13, 2023

ghost commented Mar 13, 2023

ghost commented Mar 13, 2023

peter-d commented Mar 13, 2023

timsnyder Mar 11, 2024

Choose a reason for hiding this comment

peter-d Mar 12, 2024

Choose a reason for hiding this comment

ghost commented Feb 8, 2023 •

edited by ghost

Loading

peter-d commented Feb 9, 2023 •

edited

Loading