Skip to content
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

xmllint missing from CI jobs #601

Closed
mwaxmonsky opened this issue Oct 18, 2024 · 2 comments
Closed

xmllint missing from CI jobs #601

mwaxmonsky opened this issue Oct 18, 2024 · 2 comments
Assignees
Labels

Comments

@mwaxmonsky
Copy link
Collaborator

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 mwaxmonsky added the bug label Oct 18, 2024
@mwaxmonsky
Copy link
Collaborator Author

@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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants