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

Issue with clang format in the tests #388

Closed
jmcarcell opened this issue Mar 13, 2023 · 1 comment · Fixed by #390
Closed

Issue with clang format in the tests #388

jmcarcell opened this issue Mar 13, 2023 · 1 comment · Fixed by #390

Comments

@jmcarcell
Copy link
Member

#358 introduced a sort of hidden dependency on clang-format as now there are tests which will generate a model with clang format and the -fallback-style=llvm argument is not supported by older versions of clang-format. This breaks the nightly build for key4hep, because llvm is not a dependency. I think we can agree that the tests should run without llvm since it's not a dependency of podio. These are some solutions I can think about:

  • In the CMake step, when generating the original model save some variable to be checked later at the tests to make sure that we run the tests with the same arguments as now
  • Alternatively we could just parse the dumped model and the original one even in a python one liner and compare the results. It's a slightly weaker test but there are tests already for the generation from the model
@tmadlener
Copy link
Collaborator

This could also be fixed by making the check for discovering clang-format more robust. Currently, it just checks for the presence, but does not try to actually run something:

def get_clang_format():
"""Check if clang format is available and if so get the list of arguments to
invoke it via subprocess.Popen"""
try:
cformat_exe = subprocess.check_output(['which', 'clang-format']).strip()
return [cformat_exe, "-style=file", "-fallback-style=llvm"]
except subprocess.CalledProcessError:
print("ERROR: Cannot find clang-format executable")
print(" Please make sure it is in the PATH.")
return []

Actually trying to run clang-format with all the flags we run it later should already fix all issues, because in case that does not succeed the generation will simply not format generated code.

We actually do something like that in the cmake macros:

execute_process(COMMAND ${CLANG_FORMAT_EXE} --fallback-style OUTPUT_VARIABLE CLANG_FALLBACK_STYLE ERROR_VARIABLE CLANG_FALLBACK_STYLE_ERR)

Overall, even that doesn't cover all possibilities yet, because the contents of .clang-format are in principle LLVM version dependent, and we require probably at least 9 or 10.

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

Successfully merging a pull request may close this issue.

2 participants