Conversation
|
I going through this PR together with @pearzt, I think it looks good to me. There are two things that remain after this PR is merged: Secondly, it seems there have been problems with compiling if SPD_LOG tries to compile with exceptions. |
jplehr
left a comment
There was a problem hiding this comment.
I personally disagree with removing PGIS from the build. I understand that it is deprecated. The reason I disagree is because it is the one (only one?) tool that acts as a full end-to-end test for the library and package components.
I simply do not trust the rest of our testing enough.
If this is what we want to do, I won't get in the way, but I want to raise my concern.
I excluded PGIS mostly due to it's dependence on a very old version of Extra-P which we would need to make available on Lichtenberg or build via
IMHO, PGIS's contribution to the overall MetaCG testing is small. This is kind of anecdotal, but I can't remember a CI failure over the last months that was caused by PGIS catching an error in the library. |
e247097 to
e8e8da7
Compare
This is less about PGI's ability to uncover / detect bugs in the library and more to have something non-trivial that actually uses the library API. I don't want us to work on an API that is only covered with unit tests as I have seen an API deteriorate from usability despite best intentions, simply because no one actually used it. So, if we think that with the increase in use of the API through the additional tools and downstream / out of tree consumers, I am less concerned about this point. Note, however, that at least the out of tree consumers won't "catch" any weird decisions/directions we take/implement here. I am aware that having a larger consumer of the API does not guarantee a good API design, as I know of quirks we had in the graph lib in the presence of PGIS. |
| -DMETACG_BUILD_CGCOLLECTOR=ON | ||
| -DMETACG_BUILD_GRAPH_TOOLS=ON | ||
| -DMETACG_BUILD_CGPATCH=ON | ||
| -DCAGE_USE_METAVIRT=ON |
There was a problem hiding this comment.
What are chances that tracking devel here breaks us for no reason?
At least that is my understanding from glimpsing at the CMake for this dependency.
There was a problem hiding this comment.
I think that's indeed a risk. Maybe be should talk to @ahueck about defining a metavirt release that we could then reference. This is unrelated to the CI though, right?
There was a problem hiding this comment.
yes in the sense of the technical changes
.ci/gitlab/.gitlab-ci.yml
Outdated
| CC: clang | ||
| CXX: clang++ |
There was a problem hiding this comment.
Does this set the compilers for all the jobs? Wouldn't that ruin the matrix job purpose?
There was a problem hiding this comment.
It does set the compiler, but clang and clang++ refer to binaries in different LLVM installations, depending on which environment is active.
There was a problem hiding this comment.
I've just added another variant that uses GCC to build MetaCG
|
I also think we should build some different configurations, i.e., enable / disable some components to see if that still works as intended. |
With this PR, I propose a new, revised Gitlab-CI. These are the key changes: