Conversation
|
One thing we may want to add is the integrity check as recommended here: https://docs.codecov.com/docs/codecov-uploader Otherwise, this looks good too me. |
|
It looks like MFEM is actually NOT using the So, I guess we should switch the target for this PR to the |
pazner
force-pushed
the
codecov-new-uploader
branch
from
85218d6 to
aa7d337
Compare
| shasum -a $i -c <(curl -s "https://raw.githubusercontent.com/codecov/codecov-bash/${VERSION}/SHA${i}SUM" | grep codecov) | ||
| done | ||
| # Effectively export the coverage report. | ||
| bash codecov -n "${{ inputs.name }}" -X gcov; |
There was a problem hiding this comment.
I don't think the new uploader supports -X gcov — do we need to do anything about this?
There was a problem hiding this comment.
I see the -X FEATURE argument in the documentation: https://docs.codecov.com/docs/codecov-uploader#uploader-command-line-arguments but I'm not sure the meaning is the same.
There was a problem hiding this comment.
I tried grepping gcov in the repo of the source code for their new uploader, and it didn't look they support this option.
There was a problem hiding this comment.
Based on the description, -X gcov is supposed to "Disable uploading the file" gcov, if I understand it correctly. Maybe the pattern -X FEATURE is just inaccurate and FEATURE is just FILE/DIRECTORY?
There was a problem hiding this comment.
In the old uploader, -X gcov would disable running gcov (otherwise I think the uploader would run gcov in some circumstances).
In the new uploader, I think only -X network is supported. I don't think it does anything with gcov, so it's probably fine without the flag.
There was a problem hiding this comment.
You are probably right. In the description of the bash uploader (https://docs.codecov.com/docs/about-the-codecov-bash-uploader), the case of -X gcov is explicitly listed along with other possible values for FEATURE, so it looks like the new uploader supports just FEATURE=network.
Good point, I rebased and force pushed. It's a bit confusing that master is actually behind… |
I think we should merge |
Sounds good. I'm not sure if any other projects (besides the ones listed in the README: mfem, glvis, laghos, remhos) are using the |
|
See #4 |
3 participants
Should resolve mfem/mfem#2584