search

NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 64 forks source link

xmllint missing from CI jobs #601

Open mwaxmonsky opened 1 month ago

mwaxmonsky commented 1 month ago

Description

Current capgen unit tests all pass but there is an error message hidden in the log output

Steps to Reproduce

Run any of the current capgen tests in the CI

Additional Context

N\A

Output

Sample log file excerpt from https://github.com/NCAR/ccpp-framework/actions/runs/11220321952/job/31188229511:

-- Running: /home/runner/work/ccpp-framework/ccpp-framework/scripts/ccpp_capgen.py --host-files test_host_data.meta,test_host_mod.meta,test_host.meta --scheme-files temp_scheme_files.txt,ddt_suite_files.txt --suites ddt_suite.xml,temp_suite.xml --host-name test_host --output-root /home/runner/work/ccpp-framework/ccpp-framework/test/ct_build/ccpp --debug
-- xmllint not found, could not validate file /home/runner/work/ccpp-framework/ccpp-framework/test/capgen_test/ddt_suite.xml
mwaxmonsky commented 1 month ago

@climbfuji @nusbaume @dustinswales @peverwhee @mkavulich I noticed this in a PR review recently and then found it in another PR so this has been around for a bit.

The fix is pretty straight forward to add xmllint to the environment but is this a needed part of the ccpp_capgen.py execution process? All of the tests pass so currently it is clearly not needed but I am wondering if we should change this behavior or remove it if it isn't needed as a part of the capgen execution.

We can go in either direction but I usually lean more toward requiring it (at least in this case as it looks to be checked every time we use the capgen script) and failing if the dependency is not met and adding a flag to explicitly turn it off (ie, --disable-xmllint) or vice-versa (making it an explicit opt in ie, --lint-xml).

Any thoughts/opinions on if/how we should change the behavior?

climbfuji commented 1 month ago

I like the idea to make this an option --enable-xmllint or --disable-xmllint. Whether the default should be on or off, I am undecided. If I put on my operations hat, I'd say the default should be off. Folks preparing for operational implementation can have it turned on, but once a suite (that never changes) is in operations we don't need the check.