Add coverage build-option and coverage-report in CI#863
Add coverage build-option and coverage-report in CI#863sporksmith merged 4 commits intoshadow:devfrom
Conversation
d4f9853 to
b2e0d10
Compare
|
@robgjansen @stevenengler I'll give both of you a chance to take a look, but if you'd like to opt out of reviewing in favor of the other, feel free to remove yourself as a reviewer :) |
robgjansen
left a comment
There was a problem hiding this comment.
Consider adding a note about the obscure gotcha that you mentioned in slack.
.github/workflows/build_shadow.yml
Outdated
| if [ "${{ matrix.cc }}" = "gcc" ]; then COMPILERS="gcc g++" | ||
| elif [ "${{ matrix.cc}}" = "clang" ]; then COMPILERS="clang" | ||
| else exit 1 | ||
| fi |
There was a problem hiding this comment.
Can we accomplish this through a matrix.compilers variable with a sensible default (g++) that can then be overridden in each container spec where the name changed (to gcc-c++) rather than including this longer if statement as an override? That would cut down the length of the code required to override from 4 lines to 1 line, I think.
There was a problem hiding this comment.
This was a bit a bit awkward to handle in the matrix, and in general it'd be nice not to clutter the matrix dimensions with purely "derived" dimensions.
Ended up finding another way to dedupe this logic, though: basically created a dedicated step for figuring out these parameters and re-exporting them for subsequent steps.
There was a problem hiding this comment.
This was a bit a bit awkward to handle in the matrix, and in general it'd be nice not to clutter the matrix dimensions with purely "derived" dimensions.
Actually I just noticed the latter point is moot; the GH UI doesn't include dimensions that are only set via the 'include' stanza, such as package_manager.
I'll take one more quick look to see if I can just fold these in there; when I tried it before GH ended up creating points in the matrix instead of supplementing the existing ones, but maybe I made a mistake...
There was a problem hiding this comment.
I think what's happening is that the GH CI will only use the first matching item in the 'include' list, so we can't easily use it to set multiple, orthogonal, derived dimensions. (e.g. package_manager based on container, and cxx based on cc).
It might be nice to move package_manager out of there for that reason, and either set an environment variable or have the dependency-install steps be conditional on the container directly. I'm going to leave that for a different PR, though.
There was a problem hiding this comment.
Yeah the includes feature isn't well documented. My understanding is that if only one item doesn't match, it will add that item to the existing matrix element, but if there are >1 items that don't match, it will create a new matrix element with those items.
.github/workflows/build_shadow.yml
Outdated
| # for how these 'echo' commands work. | ||
| case "${{ matrix.cc }}" in | ||
| "gcc") | ||
| echo '::set-env name=CC:gcc' |
There was a problem hiding this comment.
Hi @stevenengler - I got the idea to use workflow commands (which I previously didn't know about) from how you added rust to the path, below. These seem to be having no effect, though. Any idea what might be going wrong?
There was a problem hiding this comment.
It looks okay to me, but maybe try with for example CC::gcc instead of CC:gcc. If that doesn't work, maybe you can only have one command per shell? Hopefully not, but other than that I don't have any ideas.
There was a problem hiding this comment.
CC::gccinstead ofCC:gcc
🤦 - that was it. Thanks for spotting it! :)
There was a problem hiding this comment.
No problem, glad it was a simple change :)
Done; added a check in CMakeLists.txt |
03dd7e7 to
5824842
Compare
No description provided.