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

Unconditionally format generated code but in CI #1085

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions src/language/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,11 @@ set_source_files_properties(${NMODL_GENERATED_SOURCES} PROPERTIES GENERATED TRUE
# .clang-format configuration file. It's important that we only try to format code if formatting was
# enabled and NMODL's .clang-format exists, otherwise clang-format will search too far up the
# directory tree and find the wrong configuration file. This can break compilation.
if(NMODL_CLANG_FORMAT OR NMODL_FORMATTING)
set(CODE_GENERATOR_OPTS -v --clang-format=${ClangFormat_EXECUTABLE})
foreach(clang_format_opt ${NMODL_ClangFormat_OPTIONS} --style=file)
list(APPEND CODE_GENERATOR_OPTS --clang-format-opts=${clang_format_opt})
endforeach()
endif()

add_custom_command(
OUTPUT ${NMODL_GENERATED_SOURCES}
COMMAND ${PYTHON_EXECUTABLE} ARGS ${CMAKE_CURRENT_SOURCE_DIR}/code_generator.py
${CODE_GENERATOR_OPTS} --base-dir ${PROJECT_BINARY_DIR}/src
COMMAND ${PYTHON_EXECUTABLE} ARGS ${CMAKE_CURRENT_SOURCE_DIR}/code_generator.py --base-dir
${PROJECT_BINARY_DIR}/src
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
DEPENDS ${CODE_GENERATOR_PY_FILES}
DEPENDS ${CODE_GENERATOR_YAML_FILES}
Expand Down
39 changes: 20 additions & 19 deletions src/language/code_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import os
from pathlib import Path, PurePath
import shutil
import stat
import subprocess
import tempfile

Expand All @@ -25,14 +24,20 @@

LOGGING_FORMAT = "%(levelname)s:%(name)s: %(message)s"
LOGGER = logging.getLogger("NMODLCodeGen")
CLANG_FORMAT_EXECUTABLE = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a pure nitpick: it's not really aclang-format executable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLANG_FORMAT_COMMAND then :)

Path(__file__).resolve().parent.parent.parent
/ "cmake"
/ "hpc-coding-conventions"
/ "bin"
/ "format"
)


class CodeGenerator(
collections.namedtuple(
"Base",
[
"base_dir",
"clang_format",
"jinja_env",
"jinja_templates_dir",
"modification_date",
Expand All @@ -48,7 +53,6 @@ class CodeGenerator(

Attributes:
base_dir: output root directory where Jinja templates are rendered
clang_format: clang-format command line if C++ files have to be formatted, `None` otherwise
py_files: list of Path objects to the Python files used by this program
yaml_files: list of Path object to YAML files describing the NMODL language
modification_date: most recent modification date of the Python and YAML files in this directory
Expand All @@ -58,15 +62,14 @@ class CodeGenerator(
temp_dir: path to the directory where to create temporary files
"""

def __new__(cls, base_dir, clang_format=None):
def __new__(cls, base_dir):
this_dir = Path(__file__).parent.resolve()
jinja_templates_dir = this_dir / "templates"
py_files = [Path(p).relative_to(this_dir) for p in this_dir.glob("*.py")]
yaml_files = [Path(p).relative_to(this_dir) for p in this_dir.glob("*.yaml")]
self = super(CodeGenerator, cls).__new__(
cls,
base_dir=base_dir,
clang_format=clang_format,
this_dir=this_dir,
jinja_templates_dir=jinja_templates_dir,
jinja_env=jinja2.Environment(
Expand Down Expand Up @@ -98,6 +101,14 @@ def __new__(cls, base_dir, clang_format=None):

return self

@property
def within_ci(self):
"""Return True if code is running in a continuous-integration environment, else otherwise"""
for varname in ["CI", "CI_PIPELINE_ID", "AZURE_ENV_NAME"]:
if varname in os.environ:
return True
return False

Comment on lines +104 to +111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering why we need it i.e. what happens if the files are formatted in CI as well? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to understand this but couldn't quite figure it out from looking at our current CMake files and CI scripts...

def jinja_template(self, path):
"""Construct a Jinja template object

Expand Down Expand Up @@ -287,9 +298,9 @@ def format_output(self, file):
language: c++ or cmake

"""
if self.language == "c++" and self.app.clang_format:
LOGGER.debug("Formatting C++ file %s", str(file))
subprocess.check_call(self.app.clang_format + ["-i", str(file)])
if self.language == "c++" and not self.app.within_ci:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A clarification question: if user doesn't have "compatible" clang format installed on his/her machine, what happens in this case? .i.e. if there is no clang-format installed or of a different version than we support, user will still be able to build the nmodl without any error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is taken care of from within the coding convention tool that wraps clang-format?

LOGGER.debug("Formatting C++ file %s", file)
subprocess.check_call([CLANG_FORMAT_EXECUTABLE, "-q", str(file)])

@property
def language(self):
Expand Down Expand Up @@ -340,22 +351,12 @@ def parse_args(args=None):
args: arguments given in CLI
"""
parser = argparse.ArgumentParser()
parser.add_argument("--clang-format", help="Path to clang-format executable")
parser.add_argument(
"--clang-format-opts", help="clang-format options", action="append"
)
parser.add_argument("--base-dir", help="output root directory")
parser.add_argument(
"-v", "--verbosity", action="count", default=0, help="increase output verbosity"
)
args = parser.parse_args(args=args)

# construct clang-format command line to use, if provided
if args.clang_format:
args.clang_format = [args.clang_format]
if args.clang_format_opts:
args.clang_format += args.clang_format_opts

# destination directory to render templates
args.base_dir = (
Path(args.base_dir).resolve()
Expand Down Expand Up @@ -383,7 +384,7 @@ def main(args=None):
args = parse_args(args)
configure_logger(args.verbosity)

codegen = CodeGenerator(clang_format=args.clang_format, base_dir=args.base_dir)
codegen = CodeGenerator(base_dir=args.base_dir)
num_tasks = 0
tasks_performed = []
for task in codegen.workload():
Expand Down
Loading