Tell cmake to add boost includes for core and mss

[peter-d]

This fixes #35

[ghost]

If we fix sparta, will this patch still be needed?

[peter-d]

Yes - the way the cmake is without these fixes is that it includes the find_package from Sparta, but does not actually add the includes as a dependency for core and mss.

If you are lucky enough to be on a system that has boost in the system include path (most linux distros with the default version of boost installed) you won't see the issue.

But anyone who needs to pass a custom boost path will either not find it (if no boost is in system path) or include another version than Sparta does.

Just noticed I need to fix the CMakeLists.txt for test/core/common too - I'll update the PR.

[ghost]

Tying this to some Sparta PRs that Peter (mostly Peter 😆 ) and I are working on:
sparcians/map#397
sparcians/map#396

[peter-d]

What's checked in here now enables Olympia to find sparta after a make install of sparta as in sparcians/map#397
sparcians/map#396

Not fully working yet - this needs some changes in sparta too (namely for the WeightedContextCounter used in Dispatch)- see PRs linked above.

To use this do:

cmake -S . -B release -DCMAKE_BUILD_TYPE=Release  -DBOOST_ROOT=$(brew --prefix)/opt/[email protected]  -DSPARTA_SEARCH_DIR=$HOME/opt/sparta
make -C release -j

(of course replace the above paths by your boost library and sparta install folder as appropriate)

Disclaimer: so far only tested on Mac Ventura 13.1 with homebrew based boost install.

[ghost]

Hey Peter, when you get a chance, can you pull master from the perf model into this branch? CI is now enabled (for Ubuntu).

[peter-d]

I've merged master into this branch, but don't see CI running on the PR.

I also locally have a commit ready fix the WeightedContextCounter include in this same branch. Is CI already running against sparta master where WeightedContextCounter is already moved? If so, I can include it here, unless you prefer that to be in a separate PR?

[ghost]

Is CI already running against sparta master where WeightedContextCounter is already moved?

Yep. Feel free to include that fix here. Thanks!

[ghost]

CI required approval. Not sure why... I'll look into that.

[peter-d]

I pushed that fix but CI fails now because it cannot find the FindSparta.cmake file I added to sparta (from 4795d0b onwards this PR depends on the corresponding PR in sparta, which is not yet in CI...

It should be installed in a location that cmake looks for by default, or we should set CMAKE_MODULE_PATH to include it. Maybe it's best to add whatever gets passed as -DSPARTA_SEARCH_DIR to the CMAKE_MODULE_PATH in order for cmake to find the file, regardless of where it is. (should work even if there is no make install and you point Olympia directly to the sparta source tree, like now in CI)

Unrelated note: it's probably not needed to build sparta in every CI run for Olympia?

[ghost]

Unrelated note: it's probably not needed to build sparta in every CI run for Olympia?

Nope. I plan on adding a GH cache for it once this settles.

[ghost]

It should be installed in a location that cmake looks for by default

Agreed. I can change the flow to 'make install' of Sparta and it might just work. 🤞🏻

[ghost]

@peter-d, can you apply this patch and see if this fixes CI?

diff --git a/.github/actions/build/entrypoint.sh b/.github/actions/build/entrypoint.sh
index bbe3c6a..537c8ae 100755
--- a/.github/actions/build/entrypoint.sh
+++ b/.github/actions/build/entrypoint.sh
@@ -26,7 +26,7 @@ if [ $? -ne 0 ]; then
     echo "ERROR: Cmake for Sparta framework failed"
     exit 1
 fi
-make -j2
+make -j2 install
 BUILD_SPARTA=$?
 if [ ${BUILD_SPARTA} -ne 0 ]; then
     echo "ERROR: build sparta FAILED!!!"

[peter-d]

In principle, after a make install in some system wide location like /usr/local or /opt we should not have to give Olympia -DSPARTA_BASE, it should be able to pick it up by default. I'll push that change.

I'll give that a shot and maybe add the -DSPARTA_BASE again in case it does not work for some reason.

Note that since it looks like none of my pushes CI runs automatically until you approve, I can't do quick turnaround here (but that's fine for me)

[ghost]

I changed the settings for the action runs. We'll see if that sticks. :)

[peter-d]

Looks like we don’t have permission for a make install to /usr/local in CI. I can change it to install elsewhere, or do you prefer to change permissions in CI?

[ghost]

🤔

Let's try a local install -- most folks might be doing that anyway.

[peter-d]

I changed the settings for the action runs. We'll see if that sticks. :)

Looks like it's still not running automatically when I push.

Unrelated question: once this is properly fixed, shall I squash all the CI changes in 1 single commit, for a cleaner history on master?

[ghost]

once this is properly fixed, shall I squash all the CI changes in 1 single commit, for a cleaner history on master?

No need. I've set up the repo to do that automatically.

[peter-d]

What version of sparta is built in CI? Looks like it is one that installs the headers and .a files but not the pipe viewer and FindSparta.cmake files. (which makes sense, I guess we build master here)

But in this PR the cmake files depend on that file being installed. So this won't be CI clean until we change that...

[ghost]

What version of sparta is built in CI? Looks like it is one that installs the headers and .a files but not the pipe viewer and FindSparta.cmake files. (which makes sense, I guess we build master here)

But in this PR the cmake files depend on that file being installed. So this won't be CI clean until we change that...

Ahhh... probably not the one in sparcians/map#397. The CI uses master (probably not the smartest right now).

Let me get with Brett and see if there are any more outstanding concerns on that PR. If not, I'll merge it and then try CI here again.

BTW, I read through GH docs and could not find a way to automatically enable CI from outside forks -- it must go through a maintainer. 🙄

[ghost]

I think we don't need the C anymore -- fixed HDF5 find package.

[peter-d]

Great! I had to add it for hdf5 indeed

[ghost]

Hey @peter-d, I've given you write access to this repo -- can you move your branch here? That way I can work on it with you.

[peter-d]

Thanks for the access. I've pushed it here
I think that means we need to also start another PR, now way to change the branch this one is attached to, I think?

[peter-d]

At the moment this is still failing in CI because it needs the sparta PR, but for now it's still running on sparta master.

[ghost]

I think that means we need to also start another PR, now way to change the branch this one is attached to, I think?

Yes, I think you're right. Please close this one and start a new one. I expect CI to fail for now until I fix it (to properly install sparta on the CI machines).

[peter-d]

👍🏻
Closing this one - will continue on a branch in this repo.