-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lcov capability #324
Add lcov capability #324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, just a typo to fix and a question below.
echo " PASS - successful completion" | ||
echo " COPY - previously compiled code was copied for new test" | ||
echo " MISS - comparison data is missing" | ||
echo " PEND - status is undertermined; test may still be queued, running, or timed out" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo should be 'undetermined'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I just fixed this.
cat >> ${tsdir}/suite.run << EOF | ||
cd ${testname_base} | ||
./icepack.build | ||
./icepack.submit | ||
./icepack.submit | tee -a ../suite.jobs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This takes the output of the submit script and redirects it to the file suite.jobs in addition to going to standard out. The tee allows you to redirect output to two places. What we're doing is creating a file with all the jobs ids. Once all the jobs are submitted, this file is parsed and each job is checked that it is no longer in the queue before doing the final step which may be to report the results or do the code coverage analysis. The key is that we need to allow all the jobs to complete before doing the final reporting (if --report or --coverage) is chosen and we capture the jobids with the tee.
At the bottom of icepack.setup (cice.setup is the same), we have
if ($report == 1) then
echo "Reporting results"
./poll_queue.csh
./results.csh
./report_results.csh
endif
if ($coverage == 1) then
echo "Generating coverage reports"
./poll_queue.csh
./report_lcov.csh
# ./report_codecov.csh
endif
poll_queue.csh is the script that checks whether the jobs are done. We only do that checking if --report or --coverage is set. Finally, just fyi, suite.jobs looks like
2865298.chadmin1.ib0.cheyenne.ucar.edu
2865301.chadmin1.ib0.cheyenne.ucar.edu
2865305.chadmin1.ib0.cheyenne.ucar.edu
...
and what is surprising is that different machines and queuing systems seem to use more or less the same syntax for jobids, so we can simply parse the jobids from each line and verify that that job is no longer in the queue. This implementation has always seemed a little dicey to me, but it has held up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for the explanation. I don't think the scripts work quite the same way on badger (e.g. I get lots of FAILs when runs are getting set up, rather than PENDs, until the suite is completely finished), but I usually don't invoke --report on the command line and haven't worried about it. If it becomes a problem on other platforms then maybe we can fix it here too.
icepack.setup
Outdated
set report_name = "${shhash}:${branch}:${machine} ${testsuite}" | ||
set use_curl = 1 | ||
#for lcov | ||
setenv PERL5LIB ~/usr/lib/perl5/site_perl/5.18.2/x86_64-linux-thread-multi/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems fishy to me... I guess this is needed for lcov ?
It might be a good idea to document the lcov installation procedure somewhere (could just be in the wiki).
I would prefer if this was a variable in the env file (I haven't checked that it could work though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fishy. codecov doesn't require any special install and looks better, but it isn't robust. lcov seems to be quite robust, but there are lots of other tradeoffs including installation issues as well as usage issues. In practice, we can only produce full lcov output on cheyenne (NCAR machine) because it's the only machine we can run the pio stuff so the above is for cheyenne. I'm still not sure whether lcov needs to be usable by a broader range of developers. We're also writing the lcov output to a github page under my username. I meant to take some notes about getting lcov to work and dropped that ball. Let me see if I can recover some of that information and put it somewhere. I'll try to test lcov on another machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a wiki page for lcov installation. @phil-blain, if you see anything that can be improved, please edit and/or let me know. https://github.com/CICE-Consortium/About-Us/wiki/Install-lcov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good. I made some minor edits for the instructions to be more shell-agnostic (I'm pretty sure it works by haven't tested it my self).
Why is is not an option to put the setenv PERL5LIB ~/usr/lib/perl5/site_perl/5.18.2/x86_64-linux-thread-multi/
line in env.cheyenne_gnu
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just going to do that, move both the setenv PATH and setenv PERL5LIB to the env file and check if that works. That's definitely a cleaner solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the implementation, so the paths are now set in the env files, and this will also hopefully mean my lcov install will work for for others on those machines. I have confirmed this works on a couple platforms and have updated the lcov install instructions.
I am going to merge this now in preparation for weekend testing. |
PR checklist
Add lcov capability
apcraig
Full test suite on cheyenne passes, https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#22b19855e02599dc100ccda78c666664a00f35a4
lcov output seems to work fine too, https://apcraig.github.io/
Bring Icepack inline with CICE in terms of code coverage tools, use lcov by default instead of codecov. Change --codecov to --coverage in icepack.settings argument list.