From b7c19b9b5b8514e5b8a59eb6bf0316386e3b27f7 Mon Sep 17 00:00:00 2001 From: "Melih Y. Yalcin" Date: Mon, 4 Jul 2022 17:06:27 +0100 Subject: [PATCH] Linter integration (#1143) Add flake8, autopep8 and mypy to our build pipeline and fix the errors from new linters --- BUILD.bazel | 21 ++-- kokoro/linux-test/test.sh | 7 +- kokoro/presubmit/build.sh | 4 + kokoro/presubmit/presubmit.sh | 48 +++++++- tools/build/python/BUILD.bazel | 6 +- tools/build/python/flake8 | 17 +++ tools/build/python/lint_flake8.py | 20 ++++ tools/build/python/lint_mypy.py | 20 ++++ .../python/{py_lint.py => lint_pylint.py} | 3 +- tools/build/python/mypy.ini | 23 ++++ tools/build/python/pep8 | 16 +++ tools/build/python/python.bzl | 58 ++++++++- tools/build/python/requirements.txt | 2 + vulkan_generator/BUILD.bazel | 3 + vulkan_generator/__init__.py | 15 +++ vulkan_generator/codegen_utils/BUILD.bazel | 3 + vulkan_generator/codegen_utils/codegen.py | 67 ++++++----- vulkan_generator/generator.py | 16 +-- vulkan_generator/handle_remapper/BUILD.bazel | 4 + vulkan_generator/handle_remapper/generator.py | 112 ++++++++++-------- vulkan_generator/main.py | 4 +- vulkan_generator/tests/BUILD.bazel | 3 + .../tests/test_command_parsing.py | 7 +- vulkan_generator/vulkan_parser/BUILD.bazel | 3 + .../vulkan_parser/basetype_parser.py | 11 +- .../vulkan_parser/bitmask_parser.py | 2 +- .../vulkan_parser/defines_parser.py | 3 + .../vulkan_parser/funcptr_parser.py | 6 + .../spirv_capabilities_parser.py | 8 +- .../vulkan_parser/spirv_extensions_parser.py | 12 +- .../vulkan_parser/spirv_parser.py | 6 +- vulkan_generator/vulkan_parser/types.py | 2 +- vulkan_generator/vulkan_utils/BUILD.bazel | 3 + 33 files changed, 406 insertions(+), 129 deletions(-) create mode 100644 tools/build/python/flake8 create mode 100644 tools/build/python/lint_flake8.py create mode 100644 tools/build/python/lint_mypy.py rename tools/build/python/{py_lint.py => lint_pylint.py} (93%) create mode 100644 tools/build/python/mypy.ini create mode 100644 tools/build/python/pep8 create mode 100644 vulkan_generator/__init__.py diff --git a/BUILD.bazel b/BUILD.bazel index e61a3c308..c15efbc6e 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -347,15 +347,24 @@ test_suite( ) test_suite( - name = "tests-vulkan-generator", + name = "lint", tests = [ - # Linting + # __BEGIN_LINT "//vulkan_generator:lint", + "//vulkan_generator/codegen_utils:lint", + "//vulkan_generator/handle_remapper:lint", "//vulkan_generator/tests:lint", "//vulkan_generator/vulkan_parser:lint", "//vulkan_generator/vulkan_utils:lint", + # __END_LINT + ], +) - # Tests +test_suite( + name = "tests-vulkan-generator", + tests = [ + # Run the linting as part of Vulkan generator tests + "//:lint", # DO NOT REMOVE! "//vulkan_generator/tests:test_vulkan_parsing", ], ) @@ -448,13 +457,7 @@ test_suite( "//replay2/handle_remapper:handle_remapper_test", "//test/integration/replay:go_default_test", "//test/integration/service:go_default_test", - "//vulkan_generator:lint", - "//vulkan_generator/codegen_utils:lint", - "//vulkan_generator/handle_remapper:lint", - "//vulkan_generator/tests:lint", "//vulkan_generator/tests:test_vulkan_parsing", - "//vulkan_generator/vulkan_parser:lint", - "//vulkan_generator/vulkan_utils:lint", # __END_TESTS ], ) diff --git a/kokoro/linux-test/test.sh b/kokoro/linux-test/test.sh index 2c6135cbd..c161f441b 100755 --- a/kokoro/linux-test/test.sh +++ b/kokoro/linux-test/test.sh @@ -66,6 +66,12 @@ function test { $@ } + # If linting fails we do not need to wait entire AGI tests +test lint + +# Test Vulkan Generator first as its much smaller and independent than other packages +test tests-vulkan-generator + # Running all the tests in one go leads to an out-of-memory error on Kokoro, hence the division in smaller test sets test tests-core test tests-gapis-api @@ -74,4 +80,3 @@ test tests-gapis-other test tests-gapir test tests-gapil test tests-general -test tests-vulkan-generator diff --git a/kokoro/presubmit/build.sh b/kokoro/presubmit/build.sh index 004a3df3a..da1753c6a 100755 --- a/kokoro/presubmit/build.sh +++ b/kokoro/presubmit/build.sh @@ -45,12 +45,16 @@ sudo apt-get install -y clang-format-6.0 # Get recent Android build tools. echo y | $ANDROID_HOME/tools/bin/sdkmanager --install 'build-tools;30.0.3' +# Python Format tool +python3 -m pip install autopep8==1.6.0 --user + # Setup environment. export ANDROID_NDK_HOME=/opt/android-ndk-r16b export BAZEL=$BUILD_ROOT/bazel/bin/bazel export BUILDIFIER=$BUILD_ROOT/tools/bin/buildifier export BUILDOZER=$BUILD_ROOT/tools/bin/buildozer export CLANG_FORMAT=clang-format-6.0 +export AUTOPEP8=~/.local/bin/autopep8 cd $SRC diff --git a/kokoro/presubmit/presubmit.sh b/kokoro/presubmit/presubmit.sh index e47aa37e3..6dbd21a63 100755 --- a/kokoro/presubmit/presubmit.sh +++ b/kokoro/presubmit/presubmit.sh @@ -18,6 +18,7 @@ BAZEL=${BAZEL:-bazel} BUILDIFIER=${BUILDIFIER:-buildifier} BUILDOZER=${BUILDOZER:-buildozer} CLANG_FORMAT=${CLANG_FORMAT:-clang-format} +AUTOPEP8=${AUTOPEP8:-autopep8} if test -t 1; then ncolors=$(tput colors) @@ -85,12 +86,41 @@ function run_buildozer() { } function run_enumerate_tests() { - TARGETS="$($BAZEL query --deleted_packages=//gapii/fuchsia --output label 'kind(".*_test rule", //...)' | sort -t: -k1,1 | awk '{print " \""$0"\","}')" + # Get all the test targets in all packages except gapii/fuschia + TARGETS=$($BAZEL query --deleted_packages=//gapii/fuchsia --output label 'kind(".*_test rule", //...)') + + # Exclude linting targets from tests + TARGETS=$(echo "$TARGETS" | grep -v ":lint_") + + # Sort the test targets alphabetically + TARGETS=$(echo "$TARGETS" | sort -t: -k1,1) + + # Indent the test target names to match the BUILD.bazel file + TARGETS=$(echo "$TARGETS" | awk '{print " \""$0"\","}') + OUT=$(mktemp) cp BUILD.bazel $OUT cat $OUT | awk -v "targets=${TARGETS//$'\n'/\\n}" 'begin {a=0} /__END_TESTS/ {a=0} { if (a==0) print $0;} /__BEGIN_TESTS/ { a=1; print targets }' > BUILD.bazel } +function run_enumerate_lints() { + # Get all the linting test targets in Vulkan generator + TARGETS=$($BAZEL query 'attr(generator_function, py_lint, //vulkan_generator/...)') + + # Exclude the sub targets to prevent repetation + TARGETS=$(echo "$TARGETS" | grep -v "_flake8" | grep -v "_mypy" | grep -v "_pylint") + + # Sort the linting targets alphabetically + TARGETS=$(echo "$TARGETS" | sort -t: -k1,1) + + # Indent the linting target names to match the BUILD.bazel file + TARGETS=$(echo "$TARGETS" | awk '{print " \""$0"\","}') + + OUT=$(mktemp) + cp BUILD.bazel $OUT + cat $OUT | awk -v "targets=${TARGETS//$'\n'/\\n}" 'begin {a=0} /__END_LINT/ {a=0} { if (a==0) print $0;} /__BEGIN_LINT/ { a=1; print targets }' > BUILD.bazel +} + function run_gazelle() { echo # TODO: figure out a way to make bazel not print anything. $BAZEL run gazelle @@ -102,15 +132,16 @@ function run_gofmt() { find . -name "*.go" | xargs $GOFMT -w } +function run_autopep8() { + $AUTOPEP8 --global-config=tools/build/python/pep8 -r --in-place vulkan_generator +} + # Ensure we are clean to start out with. check "git workspace must be clean" true # Check copyright headers check copyright-headers run_copyright_headers -# Check clang-format. -check clang-format run_clang_format - # Check buildifier. check buildifier run_buildifier @@ -120,6 +151,9 @@ check "buildozer fix" run_buildozer # Check that the //:tests target contains all tests. check "//:tests contains all tests" run_enumerate_tests +# Check that "//:lint" target contains all lints +check "//:lint contains all lints" run_enumerate_lints + # Check gazelle. check "gazelle" run_gazelle @@ -127,5 +161,11 @@ check "gazelle" run_gazelle # installed its Go SDK (as a dependency to run Gazelle). check gofmt run_gofmt +# Check python Formatter(autopep8) +check autopep8 run_autopep8 + +# Check clang-format. +check clang-format run_clang_format + echo echo "${green}All check completed successfully.$normal" diff --git a/tools/build/python/BUILD.bazel b/tools/build/python/BUILD.bazel index fc1608cbe..7cfde99c9 100644 --- a/tools/build/python/BUILD.bazel +++ b/tools/build/python/BUILD.bazel @@ -13,7 +13,11 @@ # limitations under the License. exports_files([ - "py_lint.py", + "lint_flake8.py", + "lint_mypy.py", + "lint_pylint.py", + "flake8", + "mypy.ini", "py_test.py", "pylintrc", "requirements.txt", diff --git a/tools/build/python/flake8 b/tools/build/python/flake8 new file mode 100644 index 000000000..b19476547 --- /dev/null +++ b/tools/build/python/flake8 @@ -0,0 +1,17 @@ +# Copyright (C) 2022 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +[flake8] +ignore=E501 # Max line length is already checked by pylint diff --git a/tools/build/python/lint_flake8.py b/tools/build/python/lint_flake8.py new file mode 100644 index 000000000..7e89d23e2 --- /dev/null +++ b/tools/build/python/lint_flake8.py @@ -0,0 +1,20 @@ +# Copyright (C) 2022 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys +from flake8.main import cli as flake8_cli + + +if __name__ == "__main__": + flake8_cli.main(sys.argv[1:]) diff --git a/tools/build/python/lint_mypy.py b/tools/build/python/lint_mypy.py new file mode 100644 index 000000000..9cf3efd6b --- /dev/null +++ b/tools/build/python/lint_mypy.py @@ -0,0 +1,20 @@ +# Copyright (C) 2022 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys +from mypy import main as mypy_main + + +if __name__ == "__main__": + mypy_main.main(None, sys.stdout, sys.stderr, sys.argv[1:], clean_exit=True) diff --git a/tools/build/python/py_lint.py b/tools/build/python/lint_pylint.py similarity index 93% rename from tools/build/python/py_lint.py rename to tools/build/python/lint_pylint.py index 2e9ef54d3..f5408dd39 100644 --- a/tools/build/python/py_lint.py +++ b/tools/build/python/lint_pylint.py @@ -15,5 +15,6 @@ import sys import pylint + if __name__ == "__main__": - sys.exit(pylint.run_pylint(sys.argv[1:])) + pylint.run_pylint(sys.argv[1:]) diff --git a/tools/build/python/mypy.ini b/tools/build/python/mypy.ini new file mode 100644 index 000000000..ae7a7a21e --- /dev/null +++ b/tools/build/python/mypy.ini @@ -0,0 +1,23 @@ +# Copyright (C) 2022 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +[mypy] +show_column_numbers=True +pretty=False +follow_imports=silent + +# Unless you are 110% sure, *NEVER* change this option +# It will break both the type system and import discovery of mypy +ignore_missing_imports=False diff --git a/tools/build/python/pep8 b/tools/build/python/pep8 new file mode 100644 index 000000000..07ce166b5 --- /dev/null +++ b/tools/build/python/pep8 @@ -0,0 +1,16 @@ +# Copyright (C) 2022 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +[pep8] +max-line-length = 120 diff --git a/tools/build/python/python.bzl b/tools/build/python/python.bzl index f96fcc626..64aa0568f 100644 --- a/tools/build/python/python.bzl +++ b/tools/build/python/python.bzl @@ -1,13 +1,13 @@ load("@rules_python//python:defs.bzl", _py_test = "py_test") load("@pip//:requirements.bzl", "requirement") -def py_lint(name, srcs, deps = [], args = [], **kwargs): +def _lint_pylint(name, srcs, deps = [], args = [], **kwargs): _py_test( name = name, srcs = [ - "//tools/build/python:py_lint.py", + "//tools/build/python:lint_pylint.py", ] + srcs, - main = "//tools/build/python:py_lint.py", + main = "//tools/build/python:lint_pylint.py", args = [ "--rcfile=$(location //tools/build/python:pylintrc)" ] + args + ["$(location :%s)" % x for x in srcs], @@ -20,6 +20,58 @@ def py_lint(name, srcs, deps = [], args = [], **kwargs): **kwargs ) +def _lint_mypy(name, srcs, deps = [], args = [], **kwargs): + _py_test( + name = name, + srcs = [ + "//tools/build/python:lint_mypy.py", + ] + srcs, + main = "//tools/build/python:lint_mypy.py", + args = [ + "--config-file=$(location //tools/build/python:mypy.ini)" + ] + args + ["$(location :%s)" % x for x in srcs], + python_version = "PY3", + srcs_version = "PY3", + data = [ "//tools/build/python:mypy.ini" ], + deps = deps + [ + requirement("mypy"), + ], + **kwargs + ) + +def _lint_flake8(name, srcs, deps = [], args = [], **kwargs): + _py_test( + name = name, + srcs = [ + "//tools/build/python:lint_flake8.py", + ] + srcs, + main = "//tools/build/python:lint_flake8.py", + args = [ + "--config=$(location //tools/build/python:flake8)" + ] + args + ["$(location :%s)" % x for x in srcs], + python_version = "PY3", + srcs_version = "PY3", + data = [ "//tools/build/python:flake8" ], + deps = deps + [ + requirement("flake8"), + ], + **kwargs + ) + +def py_lint(name, srcs, deps = [], args = [], **kwargs): + _lint_mypy(name + "_mypy", srcs, deps, args, **kwargs) + _lint_pylint(name + "_pylint", srcs, deps, args, **kwargs) + _lint_flake8(name + "_flake8", srcs, deps, args, **kwargs) + + native.test_suite( + name=name, + tests=[ + name + "_flake8", + name + "_mypy", + name + "_pylint", + ] + ) + def py_test(name, srcs, deps = [], args = [], **kwargs): _py_test( name = name, diff --git a/tools/build/python/requirements.txt b/tools/build/python/requirements.txt index 17673b15d..f4de65a8d 100644 --- a/tools/build/python/requirements.txt +++ b/tools/build/python/requirements.txt @@ -1,2 +1,4 @@ pytest==7.1.2 pylint==2.13.8 +mypy==0.950 +flake8==4.0.1 \ No newline at end of file diff --git a/vulkan_generator/BUILD.bazel b/vulkan_generator/BUILD.bazel index df53a9ea5..a495eb35c 100644 --- a/vulkan_generator/BUILD.bazel +++ b/vulkan_generator/BUILD.bazel @@ -47,4 +47,7 @@ py_binary( py_lint( name = "lint", srcs = glob(["*.py"]), + deps = [ + "//vulkan_generator", + ], ) diff --git a/vulkan_generator/__init__.py b/vulkan_generator/__init__.py new file mode 100644 index 000000000..c5069b202 --- /dev/null +++ b/vulkan_generator/__init__.py @@ -0,0 +1,15 @@ +# Copyright (C) 2022 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Top level package for Vulkan Generator""" diff --git a/vulkan_generator/codegen_utils/BUILD.bazel b/vulkan_generator/codegen_utils/BUILD.bazel index aefe3e548..a2f9d1df5 100644 --- a/vulkan_generator/codegen_utils/BUILD.bazel +++ b/vulkan_generator/codegen_utils/BUILD.bazel @@ -25,4 +25,7 @@ py_library( py_lint( name = "lint", srcs = glob(["*.py"]), + deps = [ + "codegen_utils", + ], ) diff --git a/vulkan_generator/codegen_utils/codegen.py b/vulkan_generator/codegen_utils/codegen.py index 9b249b90c..97c838547 100644 --- a/vulkan_generator/codegen_utils/codegen.py +++ b/vulkan_generator/codegen_utils/codegen.py @@ -20,20 +20,23 @@ import textwrap -def indent_characters(depth : int = 1) -> str: # return the correct indentation for a given indent depth + +def indent_characters(depth: int = 1) -> str: # return the correct indentation for a given indent depth indent_symbols = " " ret = "" for _ in range(depth): - ret = ret +indent_symbols + ret = ret + indent_symbols return ret -def indent_code(code: str, depth : int = 1) -> str: # return the correct indentation for a given indent depth + +def indent_code(code: str, depth: int = 1) -> str: # return the correct indentation for a given indent depth if depth <= 0: return code - return indent_code(textwrap.indent(code, indent_characters(1)), depth -1) + return indent_code(textwrap.indent(code, indent_characters(1)), depth - 1) + def generated_license_header() -> str: return textwrap.dedent(""" @@ -52,52 +55,58 @@ def generated_license_header() -> str: // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. - + """) + def comment_code(code: str, comment: str) -> str: ''' Adds a comment to your code. Makes a basic effort to be pretty. ''' # if there's no code or the code is just whitespace, we'll just return the comment if len(code) == 0 or code.isspace(): - return "/* " +comment + " */\n" + return "/* " + comment + " */\n" # one-line code snippets get commented "in line" if code.count("\n") == 0 or code.count("\n") == 1 and code[-2:-1] == "\n": - return code + " /* " +comment + " */\n" + return code + " /* " + comment + " */\n" # multi-line code snippets get a comment above the code - return "/* " +comment + " */\n" +code + return "/* " + comment + " */\n" + code + def create_function_signature(name: str, return_type: str, arguments: Optional[Dict[str, str]]) -> str: ''' Create the signature for a function, ie: int fibonacci(int n) ''' ret = return_type + " " + name + "(" - for arg_name, arg_type in arguments.items(): - ret = ret + arg_type + " " + arg_name + ", " - - # Strip the trailing comma from the last argument if arguments: + for arg_name, arg_type in arguments.items(): + ret = ret + arg_type + " " + arg_name + ", " + + # Strip the trailing comma from the last argument ret = ret[0:-2] ret = ret + ")" return ret + def create_function_declaration(name: str, return_type: str = "void", arguments: Optional[Dict[str, str]] = None) -> str: ''' Create a declaration for a function, ie: int fibonacci(int n); ''' return create_function_signature(name, return_type, arguments,) + ";" + def create_function_definition(name: str, return_type: str = "void", arguments: Optional[Dict[str, str]] = None, code: str = "") -> str: ''' Create the signature for a function, ie: int fibonacci(int n) { /* code here */ }''' - return create_function_signature(name, return_type, arguments) + " {\n" + indent_code(code) +"\n}" + return create_function_signature(name, return_type, arguments) + " {\n" + indent_code(code) + "\n}" + def create_exception_declaration(name: str, base_class: str = "std::exception") -> str: - return "class " + name + " : public " + base_class +" {};" + return "class " + name + " : public " + base_class + " {};" + def create_class_definition(name: str, public_inheritance: Optional[List[str]] = None, @@ -110,10 +119,9 @@ def create_class_definition(name: str, protected_functions: Optional[List[str]] = None, private_functions: Optional[List[str]] = None ) -> str: - ''' Create the full definition for a class including inheritance, all members, etc''' - def inheritance_helper(inheritance_type : str, inheritance : List[str]) -> str: + def inheritance_helper(inheritance_type: str, inheritance: Optional[List[str]]) -> str: if not inheritance: return "" inheritance_string = inheritance_type + " " @@ -121,10 +129,10 @@ def inheritance_helper(inheritance_type : str, inheritance : List[str]) -> str: inheritance_string = inheritance_string + base + ", " return inheritance_string - def member_helper(member_type : str, members : List[str]) -> str: + def member_helper(member_type: str, members: Optional[List[str]]) -> str: if not members: return "" - members_string = indent_characters() + member_type +":\n" + members_string = indent_characters() + member_type + ":\n" for member in members: members_string = members_string + indent_characters(2) + member + "\n" return members_string + "\n" @@ -133,11 +141,11 @@ def member_helper(member_type : str, members : List[str]) -> str: inheritance = public_inheritance or protected_inheritance or private_inheritance if inheritance: - ret = ret +" : " + ret = ret + " : " - ret = ret +inheritance_helper("public", public_inheritance) - ret = ret +inheritance_helper("protected", protected_inheritance) - ret = ret +inheritance_helper("private", private_inheritance) + ret = ret + inheritance_helper("public", public_inheritance) + ret = ret + inheritance_helper("protected", protected_inheritance) + ret = ret + inheritance_helper("private", private_inheritance) # Strip the trailing comma from the last inherited base class if inheritance: @@ -151,17 +159,14 @@ def member_helper(member_type : str, members : List[str]) -> str: if contents: ret = ret + "\n" - ret = ret +member_helper("public", public_functions) - ret = ret +member_helper("protected", protected_functions) - ret = ret +member_helper("private", private_functions) + ret = ret + member_helper("public", public_functions) + ret = ret + member_helper("protected", protected_functions) + ret = ret + member_helper("private", private_functions) - ret = ret +member_helper("public", public_members) - ret = ret +member_helper("protected", protected_members) - ret = ret +member_helper("private", private_members) + ret = ret + member_helper("public", public_members) + ret = ret + member_helper("protected", protected_members) + ret = ret + member_helper("private", private_members) ret = ret + "};\n" return ret - - - diff --git a/vulkan_generator/generator.py b/vulkan_generator/generator.py index 83bbfe067..212a06dfd 100644 --- a/vulkan_generator/generator.py +++ b/vulkan_generator/generator.py @@ -86,7 +86,7 @@ def print_vulkan_metadata(vulkan_metadata: types.VulkanMetadata) -> None: def basic_generate(target: str, output_dir: Path, - all_vulkan_types: types.AllVulkanTypes, + all_vulkan_types: types.VulkanMetadata, generate_header, generate_cpp, generate_test): @@ -95,15 +95,15 @@ def basic_generate(target: str, generate_cpp(os.path.join(output_dir, target + ".cc"), all_vulkan_types) generate_test(os.path.join(output_dir, target + "_tests.cc"), all_vulkan_types) -def generate(target: str, output_dir: Path, vulkan_xml_path: Path) -> bool: +def generate(target: str, output_dir: Path, vulkan_xml_path: Path) -> bool: """ Generator function """ vulkan_metadata = vulkan_parser.parse(vulkan_xml_path) print_vulkan_metadata(vulkan_metadata) - if output_dir == "" : + if output_dir == "": print("No output directory specified. Not generating anything.") - elif target == "" : + elif target == "": print("No generate target specified. Not generating anything.") else: os.makedirs(output_dir, exist_ok=True) @@ -111,10 +111,10 @@ def generate(target: str, output_dir: Path, vulkan_xml_path: Path) -> bool: # Switch table for generate target. Add new targets here and throw exception for unknown targets if target == "handle_remapper": basic_generate(target, output_dir, vulkan_metadata, - handle_remapper_generator.generate_handle_remapper_h, - handle_remapper_generator.generate_handle_remapper_cpp, - handle_remapper_generator.generate_handle_remapper_tests) + handle_remapper_generator.generate_handle_remapper_h, + handle_remapper_generator.generate_handle_remapper_cpp, + handle_remapper_generator.generate_handle_remapper_tests) else: - raise Exception("unknown generate target: " +target) + raise Exception("unknown generate target: " + target) return True diff --git a/vulkan_generator/handle_remapper/BUILD.bazel b/vulkan_generator/handle_remapper/BUILD.bazel index 5551bdf99..4dffe5b82 100644 --- a/vulkan_generator/handle_remapper/BUILD.bazel +++ b/vulkan_generator/handle_remapper/BUILD.bazel @@ -22,10 +22,14 @@ py_library( visibility = ["//visibility:public"], deps = [ "//vulkan_generator/codegen_utils", + "//vulkan_generator/vulkan_parser", ], ) py_lint( name = "lint", srcs = glob(["*.py"]), + deps = [ + "handle_remapper", + ], ) diff --git a/vulkan_generator/handle_remapper/generator.py b/vulkan_generator/handle_remapper/generator.py index b5c016afb..755dceb44 100644 --- a/vulkan_generator/handle_remapper/generator.py +++ b/vulkan_generator/handle_remapper/generator.py @@ -23,61 +23,71 @@ from vulkan_generator.vulkan_parser import types from vulkan_generator.codegen_utils import codegen + def handle_map_name(handle: str) -> str: return f"""{handle[0:1].lower() + handle[1:]}_handle_map_""" + def handle_count_map_name(handle: str) -> str: return f"""{handle[0:1].lower() + handle[1:]}_count_map_""" + def handle_add_name(handle: str) -> str: return f"""Add{handle[0:1].upper() + handle[1:]}Handle""" + def handle_remove_name(handle: str) -> str: return f"""Remove{handle[0:1].upper() + handle[1:]}Handle""" + def handle_remap_name(handle: str) -> str: return f"""Remap{handle[0:1].upper() + handle[1:]}Handle""" + class HandleAccessorCodeGenerator(metaclass=abc.ABCMeta): ''' Abstract base class for generating accessor code implementations ''' @abc.abstractmethod - def handle_add_code(self, handle : str) -> str: + def handle_add_code(self, handle: str) -> str: pass @abc.abstractmethod - def handle_remove_code(self, handle : str) -> str: + def handle_remove_code(self, handle: str) -> str: pass @abc.abstractmethod - def handle_remap_code(self, handle : str) -> str: + def handle_remap_code(self, handle: str) -> str: pass + class DispatchableHandleAccessorCodeGenerator(HandleAccessorCodeGenerator): ''' Class for generating accessor code implementations for dispatchable handles''' - def handle_add_code(self, handle : str) -> str: + + def handle_add_code(self, handle: str) -> str: map_name = handle_map_name(handle) return dedent(f""" if({map_name}.find(captureHandle) != {map_name}.end()) throw HandleCollisionException(); {map_name}[captureHandle] = replayHandle;""" - ) + ) - def handle_remove_code(self, handle : str) -> str: + def handle_remove_code(self, handle: str) -> str: map_name = handle_map_name(handle) return dedent(f""" if({map_name}.find(captureHandle) == {map_name}.end()) throw RemoveNonExistantHandleException(); {map_name}.erase(captureHandle);""" - ) + ) - def handle_remap_code(self, handle : str) -> str: + def handle_remap_code(self, handle: str) -> str: map_name = handle_map_name(handle) return dedent(f""" if({map_name}.find(captureHandle) == {map_name}.end()) throw RemapNonExistantHandleException(); return {map_name}[captureHandle];""" - ) + ) + class NonDispatchableHandleAccessorCodeGenerator(HandleAccessorCodeGenerator): ''' Class for generating accessor code implementations for non-dispatchable handles''' - def handle_add_code(self, handle : str) -> str: + + def handle_add_code(self, handle: str) -> str: map_name = handle_map_name(handle) map_count_name = handle_count_map_name(handle) return dedent(f""" @@ -94,9 +104,9 @@ def handle_add_code(self, handle : str) -> str: {map_name}[captureHandle] = replayHandle; {map_count_name}[captureHandle]++;""" - ) + ) - def handle_remove_code(self, handle : str) -> str: + def handle_remove_code(self, handle: str) -> str: map_name = handle_map_name(handle) map_count_name = handle_count_map_name(handle) return dedent(f""" @@ -114,9 +124,9 @@ def handle_remove_code(self, handle : str) -> str: {map_name}.erase(map_iter); {map_count_name}.erase(map_count_iter); }}""" - ) + ) - def handle_remap_code(self, handle : str) -> str: + def handle_remap_code(self, handle: str) -> str: map_name = handle_map_name(handle) map_count_name = handle_count_map_name(handle) return dedent(f""" @@ -131,9 +141,10 @@ def handle_remap_code(self, handle : str) -> str: }} return map_iter->second;""" - ) + ) + -def generate_handle_remapper_h(file_path : Path, vulkan_metadata : types.VulkanMetadata) : +def generate_handle_remapper_h(file_path: Path, vulkan_metadata: types.VulkanMetadata): ''' Generates handle_remapper.h ''' with open(file_path, "w", encoding="ascii") as remapper_h: @@ -152,14 +163,14 @@ def generate_handle_remapper_h(file_path : Path, vulkan_metadata : types.VulkanM """)) - private_members : List[str] = [] - public_members : List[str] = [] - public_functions : List[str] = [] + private_members: List[str] = [] + public_members: List[str] = [] + public_functions: List[str] = [] public_members.append(codegen.create_exception_declaration("InternalConsistencyException")) public_members.append(codegen.create_exception_declaration("HandleCollisionException")) public_members.append(codegen.create_exception_declaration("NonDispatchableHandleRedefinitionException", - base_class = "HandleCollisionException")) + base_class="HandleCollisionException")) public_members.append(codegen.create_exception_declaration("RemoveNonExistantHandleException")) public_members.append(codegen.create_exception_declaration("RemapNonExistantHandleException")) @@ -173,21 +184,21 @@ def generate_handle_remapper_h(file_path : Path, vulkan_metadata : types.VulkanM private_members.append("") public_functions.append(codegen.create_function_declaration(handle_add_name(handle), - arguments = {"captureHandle" : "VulkanHandle", - "replayHandle" : "VulkanHandle"})) + arguments={"captureHandle": "VulkanHandle", + "replayHandle": "VulkanHandle"})) public_functions.append(codegen.create_function_declaration(handle_remove_name(handle), - arguments = {"captureHandle" : "VulkanHandle"})) + arguments={"captureHandle": "VulkanHandle"})) public_functions.append(codegen.create_function_declaration(handle_remap_name(handle), - return_type = "VulkanHandle", - arguments = {"captureHandle" : "VulkanHandle"})) + return_type="VulkanHandle", + arguments={"captureHandle": "VulkanHandle"})) public_functions.append("") remapper_class_def = codegen.create_class_definition("VulkanHandleRemapper", - public_inheritance = ["non_copyable"], - public_functions = public_functions, - public_members = public_members, - private_members = private_members) - remapper_h.write(remapper_class_def +"\n") + public_inheritance=["non_copyable"], + public_functions=public_functions, + public_members=public_members, + private_members=private_members) + remapper_h.write(remapper_class_def + "\n") remapper_h.write(dedent(""" } @@ -195,7 +206,8 @@ def generate_handle_remapper_h(file_path : Path, vulkan_metadata : types.VulkanM """)) -def generate_handle_remapper_cpp(file_path : Path, vulkan_metadata : types.VulkanMetadata) : + +def generate_handle_remapper_cpp(file_path: Path, vulkan_metadata: types.VulkanMetadata): ''' Generates handle_remapper.cc ''' with open(file_path, "w", encoding="ascii") as remapper_cpp: @@ -218,28 +230,28 @@ def generate_handle_remapper_cpp(file_path : Path, vulkan_metadata : types.Vulka implgenerator = dispatchable_implgen if dispatchable else nondispatchable_implgen add_definition = codegen.create_function_definition( - f"""VulkanHandleRemapper::{handle_add_name(handle)}""", - arguments = {"captureHandle" : "VulkanHandle", - "replayHandle" : "VulkanHandle"}, - code = implgenerator.handle_add_code(handle)) + f"""VulkanHandleRemapper::{handle_add_name(handle)}""", + arguments={"captureHandle": "VulkanHandle", + "replayHandle": "VulkanHandle"}, + code=implgenerator.handle_add_code(handle)) remove_definition = codegen.create_function_definition( - f"""VulkanHandleRemapper::{handle_remove_name(handle)}""", - arguments = {"captureHandle" : "VulkanHandle"}, - code = implgenerator.handle_remove_code(handle)) + f"""VulkanHandleRemapper::{handle_remove_name(handle)}""", + arguments={"captureHandle": "VulkanHandle"}, + code=implgenerator.handle_remove_code(handle)) remap_definition = codegen.create_function_definition( - f"""VulkanHandleRemapper::{handle_remap_name(handle)}""", - arguments = {"captureHandle" : "VulkanHandle"}, - return_type = "VulkanHandle", - code = implgenerator.handle_remap_code(handle)) - - remapper_cpp.write(codegen.comment_code(code = add_definition, - comment = "record the creation of a new handle") +"\n") - remapper_cpp.write(codegen.comment_code(code = remove_definition, - comment = "record the destruction of a handle") +"\n") - remapper_cpp.write(codegen.comment_code(code = remap_definition, - comment = "remap a handle from the capture to replay handle") +"\n") + f"""VulkanHandleRemapper::{handle_remap_name(handle)}""", + arguments={"captureHandle": "VulkanHandle"}, + return_type="VulkanHandle", + code=implgenerator.handle_remap_code(handle)) + + remapper_cpp.write(codegen.comment_code(code=add_definition, + comment="record the creation of a new handle") + "\n") + remapper_cpp.write(codegen.comment_code(code=remove_definition, + comment="record the destruction of a handle") + "\n") + remapper_cpp.write(codegen.comment_code(code=remap_definition, + comment="remap a handle from the capture to replay handle") + "\n") remapper_cpp.write(dedent(""" } @@ -248,7 +260,7 @@ def generate_handle_remapper_cpp(file_path : Path, vulkan_metadata : types.Vulka """)) -def generate_handle_remapper_tests(file_path : Path, vulkan_metadata : types.VulkanMetadata) : +def generate_handle_remapper_tests(file_path: Path, vulkan_metadata: types.VulkanMetadata): ''' Generates handle_remapper_tests.cc ''' with open(file_path, "w", encoding="ascii") as tests_cpp: diff --git a/vulkan_generator/main.py b/vulkan_generator/main.py index 13d5311cd..7c6a350b3 100644 --- a/vulkan_generator/main.py +++ b/vulkan_generator/main.py @@ -23,9 +23,9 @@ def main() -> None: """ Entry point """ if len(sys.argv) == 4: - generator.generate(target = sys.argv[1], output_dir = Path(sys.argv[2]), vulkan_xml_path = Path(sys.argv[3])) + generator.generate(target=sys.argv[1], output_dir=Path(sys.argv[2]), vulkan_xml_path=Path(sys.argv[3])) elif len(sys.argv) == 2: - generator.generate("", "", sys.argv[1]) + generator.generate("", Path(), Path(sys.argv[1])) else: print(""" Please run this as one of the following: diff --git a/vulkan_generator/tests/BUILD.bazel b/vulkan_generator/tests/BUILD.bazel index 876898df9..ab0227e6a 100644 --- a/vulkan_generator/tests/BUILD.bazel +++ b/vulkan_generator/tests/BUILD.bazel @@ -25,4 +25,7 @@ py_test( py_lint( name = "lint", srcs = glob(["*.py"]), + deps = [ + "test_vulkan_parsing", + ], ) diff --git a/vulkan_generator/tests/test_command_parsing.py b/vulkan_generator/tests/test_command_parsing.py index 59509b7a0..9e00d0d9c 100644 --- a/vulkan_generator/tests/test_command_parsing.py +++ b/vulkan_generator/tests/test_command_parsing.py @@ -75,10 +75,12 @@ def test_vulkan_command_with_success_and_error_code() -> None: assert command.return_type == "VkResult" - assert len(command.error_codes) == 2 + assert command.error_codes + assert command.error_codes and len(command.error_codes) == 2 assert "VK_ERROR_OUT_OF_HOST_MEMORY" in command.error_codes assert "VK_ERROR_OUT_OF_DEVICE_MEMORY" in command.error_codes + assert command.success_codes assert len(command.success_codes) == 2 assert "VK_SUCCESS" in command.success_codes assert "VK_INCOMPLETE" in command.success_codes @@ -116,6 +118,7 @@ def test_vulkan_command_with_command_buffer_levels() -> None: typ = commands_parser.parse(ET.fromstring(xml)) command = typ.commands["vkCmdSetLineWidth"] + assert command.command_buffer_levels assert len(command.command_buffer_levels) == 2 assert "primary" in command.command_buffer_levels assert "secondary" in command.command_buffer_levels @@ -137,6 +140,8 @@ def test_vulkan_command_with_queues() -> None: typ = commands_parser.parse(ET.fromstring(xml)) command = typ.commands["vkCmdExecuteCommands"] + + assert command.queues assert len(command.queues) == 3 assert "transfer" in command.queues assert "graphics" in command.queues diff --git a/vulkan_generator/vulkan_parser/BUILD.bazel b/vulkan_generator/vulkan_parser/BUILD.bazel index f51ba643e..1d87ae61f 100644 --- a/vulkan_generator/vulkan_parser/BUILD.bazel +++ b/vulkan_generator/vulkan_parser/BUILD.bazel @@ -28,4 +28,7 @@ py_library( py_lint( name = "lint", srcs = glob(["*.py"]), + deps = [ + "vulkan_parser", + ], ) diff --git a/vulkan_generator/vulkan_parser/basetype_parser.py b/vulkan_generator/vulkan_parser/basetype_parser.py index 7194e0525..63a775b9c 100644 --- a/vulkan_generator/vulkan_parser/basetype_parser.py +++ b/vulkan_generator/vulkan_parser/basetype_parser.py @@ -38,10 +38,11 @@ def parse(basetype_elem: ET.Element) -> types.VulkanBaseType: name = parsing_utils.get_text_from_tag_in_children(basetype_elem, "name") basetype = parsing_utils.try_get_text_from_tag_in_children(basetype_elem, "type") - # If basetype is a pointer, pointer attribute is in the tail of the type - basetype_attribute = parsing_utils.try_get_tail_from_tag_in_children(basetype_elem, "type") - if basetype_attribute: - basetype_attribute = parsing_utils.clean_type_string(basetype_attribute) - basetype += basetype_attribute + if basetype: + # If basetype is a pointer, pointer attribute is in the tail of the type + basetype_attribute = parsing_utils.try_get_tail_from_tag_in_children(basetype_elem, "type") + if basetype_attribute: + basetype_attribute = parsing_utils.clean_type_string(basetype_attribute) + basetype = f"{basetype}{basetype_attribute}" return types.VulkanBaseType(typename=name, basetype=basetype) diff --git a/vulkan_generator/vulkan_parser/bitmask_parser.py b/vulkan_generator/vulkan_parser/bitmask_parser.py index f9db821ab..71e8e7c45 100644 --- a/vulkan_generator/vulkan_parser/bitmask_parser.py +++ b/vulkan_generator/vulkan_parser/bitmask_parser.py @@ -45,7 +45,7 @@ def parse_bitmask_by_attribute(bitmask_elem: ET.Element) -> types.VulkanBitmaskA return types.VulkanBitmaskAlias(typename=name, aliased_typename=alias) -def parse_bitmask_by_tag(bitmask_elem: ET.Element) -> types.VulkanBitmaskAlias: +def parse_bitmask_by_tag(bitmask_elem: ET.Element) -> types.VulkanBitmask: bitmask_name = parsing_utils.get_text_from_tag_in_children(bitmask_elem, "name") # This is optional because there are flags that are not used but reserved for the future. bitfield_type = parsing_utils.try_get_attribute(bitmask_elem, "requires") diff --git a/vulkan_generator/vulkan_parser/defines_parser.py b/vulkan_generator/vulkan_parser/defines_parser.py index 388e4bb32..92334263a 100644 --- a/vulkan_generator/vulkan_parser/defines_parser.py +++ b/vulkan_generator/vulkan_parser/defines_parser.py @@ -30,6 +30,9 @@ def parse_define_by_attribute(define_elem: ET.Element) -> types.VulkanDefine: name = define_elem.attrib["name"] value = define_elem.text + if not value: + raise SyntaxError(f"Vulkan define does not have definition: {ET.tostring(define_elem, 'utf-8')!r}") + return types.VulkanDefine( variable_name=name, value=value diff --git a/vulkan_generator/vulkan_parser/funcptr_parser.py b/vulkan_generator/vulkan_parser/funcptr_parser.py index 35907d5f4..1e76a2613 100644 --- a/vulkan_generator/vulkan_parser/funcptr_parser.py +++ b/vulkan_generator/vulkan_parser/funcptr_parser.py @@ -35,6 +35,12 @@ def parse_arguments(function_ptr_elem: ET.Element) -> OrderedDict[str, types.Vul if elem.tag != "type": continue + if not elem.text: + raise SyntaxError(f"Argument type could not found: {ET.tostring(elem, 'utf-8')!r}") + + if not elem.tail: + raise SyntaxError(f"Argument name could not found: {ET.tostring(elem, 'utf-8')!r}") + argument_type = parsing_utils.clean_type_string(elem.text) argument_name = parsing_utils.clean_type_string(elem.tail) diff --git a/vulkan_generator/vulkan_parser/spirv_capabilities_parser.py b/vulkan_generator/vulkan_parser/spirv_capabilities_parser.py index a54163ae9..7ed0178f9 100644 --- a/vulkan_generator/vulkan_parser/spirv_capabilities_parser.py +++ b/vulkan_generator/vulkan_parser/spirv_capabilities_parser.py @@ -20,7 +20,7 @@ from vulkan_generator.vulkan_parser import types -def parse(spriv_element: ET.Element) -> types.SpirvCapability: +def parse(spirv_element: ET.Element) -> types.SpirvCapability: """Parses a Spirv capability or alias from the XML element that defines it A sample spirv capability: @@ -32,13 +32,13 @@ def parse(spriv_element: ET.Element) -> types.SpirvCapability: """ - name = spriv_element.attrib["name"] + name = spirv_element.attrib["name"] version: Optional[str] = None feature: Optional[str] = None vulkan_property: Optional[str] = None extension: Optional[str] = None - for enable in spriv_element: + for enable in spirv_element: if "version" in enable.attrib: version = enable.attrib["version"] elif "struct" in enable.attrib: @@ -50,7 +50,7 @@ def parse(spriv_element: ET.Element) -> types.SpirvCapability: elif "extension" in enable.attrib: extension = enable.attrib["extension"] else: - raise SyntaxError(f"Unknown Spirv capability type: {ET.tostring(spriv_element, 'utf-8')}") + raise SyntaxError(f"Unknown Spirv capability type: {ET.tostring(spirv_element, 'utf-8')}") return types.SpirvCapability( name=name, diff --git a/vulkan_generator/vulkan_parser/spirv_extensions_parser.py b/vulkan_generator/vulkan_parser/spirv_extensions_parser.py index 4ad5b2995..12385332e 100644 --- a/vulkan_generator/vulkan_parser/spirv_extensions_parser.py +++ b/vulkan_generator/vulkan_parser/spirv_extensions_parser.py @@ -20,7 +20,7 @@ from vulkan_generator.vulkan_parser import types -def parse(spriv_element: ET.Element) -> types.SpirvExtension: +def parse(spirv_element: ET.Element) -> types.SpirvExtension: """Returns a Spirv extension or alias from the XML element that defines it A sample Spirv extension: @@ -30,16 +30,20 @@ def parse(spriv_element: ET.Element) -> types.SpirvExtension: """ - name = spriv_element.attrib["name"] + name = spirv_element.attrib["name"] version: Optional[str] = None extension: Optional[str] = None - for enable in spriv_element: + for enable in spirv_element: if "version" in enable.attrib: version = enable.attrib["version"] elif "extension" in enable.attrib: extension = enable.attrib["extension"] else: - raise SyntaxError(f"Unknown Spirv capability type: {ET.tostring(spriv_element, 'utf-8')}") + raise SyntaxError(f"Unknown Spirv capability type: {ET.tostring(spirv_element, 'utf-8')}") + + if not extension: + raise SyntaxError( + f"Vulkan extension for the Spirv extension could not found:{ET.tostring(spirv_element, 'utf-8')!r}") return types.SpirvExtension(name=name, version=version, extension=extension) diff --git a/vulkan_generator/vulkan_parser/spirv_parser.py b/vulkan_generator/vulkan_parser/spirv_parser.py index 9a81cafb8..c2d2e3a6d 100644 --- a/vulkan_generator/vulkan_parser/spirv_parser.py +++ b/vulkan_generator/vulkan_parser/spirv_parser.py @@ -23,11 +23,11 @@ def process_spirv_extensions(spirv_metadata: types.SpirvMetadata, spirv_extensions_element: ET.Element) -> None: """Process all the spirv extensions parsed from the Vulkan XML""" - for child in spirv_extensions_element: - spirv_extension = spirv_extensions_parser.parse(child) + for extension_element in spirv_extensions_element: + spirv_extension = spirv_extensions_parser.parse(extension_element) if not spirv_extension: - raise SyntaxError(f"Spirv Extension could not found: {ET.tostring(spirv_extension, 'utf-8')}") + raise SyntaxError(f"Spirv Extension could not found: {ET.tostring(extension_element, 'utf-8')}") spirv_metadata.extensions[spirv_extension.name] = spirv_extension diff --git a/vulkan_generator/vulkan_parser/types.py b/vulkan_generator/vulkan_parser/types.py index 0ec7c0cb6..0e14dc738 100644 --- a/vulkan_generator/vulkan_parser/types.py +++ b/vulkan_generator/vulkan_parser/types.py @@ -354,7 +354,7 @@ class SpirvCapability: property: Optional[str] # Vulkan extension enabled by this Spirv extension - extension: str + extension: Optional[str] @dataclass diff --git a/vulkan_generator/vulkan_utils/BUILD.bazel b/vulkan_generator/vulkan_utils/BUILD.bazel index 44eb471bc..86b4a1c1e 100644 --- a/vulkan_generator/vulkan_utils/BUILD.bazel +++ b/vulkan_generator/vulkan_utils/BUILD.bazel @@ -25,4 +25,7 @@ py_library( py_lint( name = "lint", srcs = glob(["*.py"]), + deps = [ + "vulkan_utils", + ], )