From f7abb161e1078f31c330f8f0fbcb41ed295df6a8 Mon Sep 17 00:00:00 2001 From: Zapta Date: Tue, 26 Nov 2024 12:53:01 -0800 Subject: [PATCH 1/9] Redirected the 'examples' package loading from zapta back to FPGAwars. --- apio/resources/packages.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apio/resources/packages.json b/apio/resources/packages.json index 58afc98a..7c25552d 100644 --- a/apio/resources/packages.json +++ b/apio/resources/packages.json @@ -2,7 +2,7 @@ "examples": { "repository": { "name": "apio-examples", - "organization": "zapta" + "organization": "FPGAwars" }, "release": { "tag_name": "%V", @@ -10,7 +10,7 @@ "uncompressed_name": "apio-examples-%V", "folder_name": "examples", "extension": "zip", - "url_version": "https://github.com/zapta/apio-examples/raw/master/VERSION" + "url_version": "https://github.com/FPGAwars/apio-examples/raw/master/VERSION" }, "description": "Verilog examples", "env": {} From 6ca2f45b642db8fe88fc9441a8be9e4a413d4f3c Mon Sep 17 00:00:00 2001 From: Zapta Date: Tue, 26 Nov 2024 19:19:22 -0800 Subject: [PATCH 2/9] Added test_scons_util.py with tests of a few functions in scons_util.py. More will be added in the future. --- .gitignore | 1 + .vscode/launch.json | 2 +- apio/scons/__init__.py | 0 apio/scons/ecp5/SConstruct | 4 +- apio/scons/gowin/SConstruct | 4 +- apio/scons/ice40/SConstruct | 4 +- apio/scons/scons_util.py | 74 +++++++++++++++++------------- test/test_scons_util.py | 89 +++++++++++++++++++++++++++++++++++++ 8 files changed, 140 insertions(+), 38 deletions(-) create mode 100644 apio/scons/__init__.py create mode 100644 test/test_scons_util.py diff --git a/.gitignore b/.gitignore index 1d5ba4c2..e7308766 100644 --- a/.gitignore +++ b/.gitignore @@ -40,3 +40,4 @@ hardware.svg abc.history temp-* .run.sh +,coverage.* \ No newline at end of file diff --git a/.vscode/launch.json b/.vscode/launch.json index 809ee6af..d317f68b 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -300,7 +300,7 @@ "module": "pytest", "args": [ "-s", - "test/commands/test_build.py" + "test/test_scons_util.py" ], "console": "integratedTerminal", "justMyCode": false, diff --git a/apio/scons/__init__.py b/apio/scons/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/apio/scons/ecp5/SConstruct b/apio/scons/ecp5/SConstruct index 6ac47705..fb45b6bd 100644 --- a/apio/scons/ecp5/SConstruct +++ b/apio/scons/ecp5/SConstruct @@ -50,7 +50,7 @@ from apio.scons.scons_util import ( TARGET, BUILD_DIR, SConstructId, - is_testbench, + has_testbench_name, basename, is_windows, get_constraint_file, @@ -240,7 +240,7 @@ def iverilog_tb_generator(source, target, env, for_signature): for a given testbench target.""" # Extract testbench name from target file name. testbench_file = str(target[0]) - assert is_testbench(env, testbench_file), testbench_file + assert has_testbench_name(env, testbench_file), testbench_file testbench_name = basename(env, testbench_file) # Construct the command line. diff --git a/apio/scons/gowin/SConstruct b/apio/scons/gowin/SConstruct index 5dc73638..b31ef661 100644 --- a/apio/scons/gowin/SConstruct +++ b/apio/scons/gowin/SConstruct @@ -49,7 +49,7 @@ from SCons.Script import ( from apio.scons.scons_util import ( TARGET, SConstructId, - is_testbench, + has_testbench_name, basename, is_windows, get_constraint_file, @@ -232,7 +232,7 @@ def iverilog_tb_generator(source, target, env, for_signature): for a given testbench target.""" # Extract testbench name from target file name. testbench_file = str(target[0]) - assert is_testbench(env, testbench_file), testbench_file + assert has_testbench_name(env, testbench_file), testbench_file testbench_name = basename(env, testbench_file) # Construct the command line. diff --git a/apio/scons/ice40/SConstruct b/apio/scons/ice40/SConstruct index 205e9bdb..2d54f328 100644 --- a/apio/scons/ice40/SConstruct +++ b/apio/scons/ice40/SConstruct @@ -49,7 +49,7 @@ from SCons.Script import ( from apio.scons.scons_util import ( TARGET, SConstructId, - is_testbench, + has_testbench_name, basename, is_windows, get_constraint_file, @@ -257,7 +257,7 @@ def iverilog_tb_generator(source, target, env, for_signature): for a given testbench target.""" # Extract testbench name from target file name. testbench_file = str(target[0]) - assert is_testbench(env, testbench_file), testbench_file + assert has_testbench_name(env, testbench_file), testbench_file testbench_name = basename(env, testbench_file) # Construct the command line. diff --git a/apio/scons/scons_util.py b/apio/scons/scons_util.py index 2700434e..f063b96b 100644 --- a/apio/scons/scons_util.py +++ b/apio/scons/scons_util.py @@ -21,7 +21,7 @@ import re from enum import Enum import json -from typing import Dict, Tuple, List, Optional +from typing import Dict, Tuple, List, Optional, NoReturn from dataclasses import dataclass import click from SCons import Scanner @@ -87,12 +87,15 @@ def is_verilog_src(env: SConsEnvironment, file_name: str) -> str: """Given a file name, determine by its extension if it's a verilog source file (testbenches included).""" _, ext = os.path.splitext(file_name) - return ext == "v" + return ext == ".v" -def is_testbench(env: SConsEnvironment, file_name: str) -> bool: - """Given a file name, return true if it's a testbench file.""" - name = basename(env, file_name) +def has_testbench_name(env: SConsEnvironment, file_name: str) -> bool: + """Given a file name, return true if it's base name indicates a + testbench. It can be for example abc_tb.v or _build/abc_tb.out. + The file extension is ignored. + """ + name, _ = os.path.splitext(file_name) return name.lower().endswith("_tb") @@ -197,6 +200,9 @@ def force_colors(env: SConsEnvironment) -> bool: def msg(env: SConsEnvironment, text: str, fg: str = None) -> None: + """Print a message to the user. Similar to click.secho but with + proper color enforcement. + """ click.secho(text, fg=fg, color=force_colors(env)) @@ -215,7 +221,7 @@ def error(env: SConsEnvironment, text: str) -> None: msg(env, f"Error: {text}", fg="red") -def fatal_error(env: SConsEnvironment, text: str) -> None: +def fatal_error(env: SConsEnvironment, text: str) -> NoReturn: """Prints a short error message and exit with an error code.""" error(env, text) env.Exit(1) @@ -311,7 +317,7 @@ def get_programmer_cmd(env: SConsEnvironment) -> str: return prog_arg -def make_verilog_src_scanner(env: SConsEnvironment) -> Scanner: +def make_verilog_src_scanner(env: SConsEnvironment) -> Scanner.Base: """Creates and returns a scons Scanner object for scanning verilog files for dependencies. """ @@ -344,6 +350,7 @@ def verilog_src_scanner_func( ".v" ), f"Not a .v file: {file_node.name}" includes_set = set() + # If the file doesn't exist, this returns an empty string. file_text = file_node.get_text_contents() # Get IceStudio includes. includes = icestudio_list_re.findall(file_text) @@ -351,7 +358,7 @@ def verilog_src_scanner_func( # Get Standard verilog includes. includes = verilog_include_re.findall(file_text) includes_set.update(includes) - # Get a deterministic list. + # Get a deterministic list. (Does it sort by file.name?) includes_list = sorted(list(includes_set)) # For debugging # info(env, f"*** {file_node.name} includes {includes_list}") @@ -461,7 +468,7 @@ def get_source_files(env: SConsEnvironment) -> Tuple[List[str], List[str]]: synth_srcs = [] test_srcs = [] for file in files: - if is_testbench(env, file.name): + if has_testbench_name(env, file.name): test_srcs.append(file.name) else: synth_srcs.append(file.name) @@ -662,6 +669,7 @@ def make_verilator_action( return action +# pylint: disable=too-many-locals def _print_pnr_report( env: SConsEnvironment, json_txt: str, @@ -696,6 +704,7 @@ def _print_pnr_report( clocks = report["fmax"] if len(clocks) > 0: for clk_net, vals in clocks.items(): + # pylint: disable=fixme # TODO: Confirm clk name extraction for Gowin. # Extract clock name from the net name. if script_id == SConstructId.SCONSTRUCT_ECP5: @@ -738,28 +747,30 @@ def print_pnr_report( return Action(print_pnr_report, "Formatting pnr report.") -def wait_for_remote_debugger(env: SConsEnvironment): - """For developement only. Useful for debugging SConstruct scripts that apio - runs as a subprocesses. Call this from the SCconstruct script, run apio - from a command line, and then connect with the Visual Studio Code debugger - using the launch.json debug target. Can also be used to debug apio itself, - without having to create or modify the Visual Studio Code debug targets - in launch.json""" - - # -- We require this import only when using the debugger. - import debugpy - - # -- 5678 is the default debugger port. - port = 5678 - msg( - env, - f"Waiting for remote debugger on port localhost:{port}.", - fg="magenta", - ) - debugpy.listen(port) - msg(env, "Attach with the Visual Studio Code debugger.") - debugpy.wait_for_client() - msg(env, "Remote debugger is attached.", fg="green") +# Enable for debugging a scons process and call from SConstruct. +# +# def wait_for_remote_debugger(env: SConsEnvironment): +# """For developement only. Useful for debugging SConstruct scripts that +# apio runs as a subprocesses. Call this from the SCconstruct script, run +# apio from a command line, and then connect with the Visual Studio Code +# debugger using the launch.json debug target. Can also be used to debug +# apio itself, without having to create or modify the Visual Studio Code +# debug targets in launch.json""" + +# # -- We require this import only when using the debugger. +# import debugpy + +# # -- 5678 is the default debugger port. +# port = 5678 +# msg( +# env, +# f"Waiting for remote debugger on port localhost:{port}.", +# fg="magenta", +# ) +# debugpy.listen(port) +# msg(env, "Attach with the Visual Studio Code debugger.") +# debugpy.wait_for_client() +# msg(env, "Remote debugger is attached.", fg="green") def set_up_cleanup(env: SConsEnvironment) -> None: @@ -780,6 +791,7 @@ def set_up_cleanup(env: SConsEnvironment) -> None: + env.Glob("_build") ) + # pylint: disable=fixme # -- TODO: Remove the cleanup of legacy files after releasing the first # -- release with the _build directory. # -- diff --git a/test/test_scons_util.py b/test/test_scons_util.py new file mode 100644 index 00000000..5271ea61 --- /dev/null +++ b/test/test_scons_util.py @@ -0,0 +1,89 @@ +""" +Tests of scons_util.py +""" + +from test.conftest import ApioRunner +from SCons.Script import DefaultEnvironment +from SCons.Script.SConscript import SConsEnvironment +from SCons.Node.FS import FS, File +from apio.scons.scons_util import ( + make_verilog_src_scanner, + has_testbench_name, + is_verilog_src, +) + +DEPENDECIES_TEST_TEXT = """ +// Test file +parameter v771499 = "v771499.list" +`include "apio_testing.vh" +parameter v771499 = "v771499.list" +`include "apio_testing.v +""" + + +def test_dependencies(apio_runner: ApioRunner): + """Test the verilog scanner which scans a verilog file and extract + reference of files it uses. + """ + + with apio_runner.in_disposable_temp_dir(): + + # -- Write a test file name in the current directory. + with open("test_file.v", "w", encoding="utf-8") as f: + f.write(DEPENDECIES_TEST_TEXT) + + # -- Create a scanner + env: SConsEnvironment = DefaultEnvironment(ENV={}, tools=[]) + scanner = make_verilog_src_scanner(env) + + # -- Run the scanner. It returns a list of File. + file = FS.File(FS(), "test_file.v") + dependency_files = scanner.function(file, env, None) + + # -- Create a list with file name strings. + file_names = [] + for f in dependency_files: + assert isinstance(f, File) + file_names.append(f.name) + + # -- Check the list. The scanner returns the files sorted and + # -- with dulicates removed. + assert file_names == ["apio_testing.vh", "v771499.list"] + + +def test_has_testbench_name(): + """Tests the scons_util.test_is_testbench() function""" + + env: SConsEnvironment = DefaultEnvironment(ENV={}, tools=[]) + + # -- Testbench names + assert has_testbench_name(env, "aaa_tb.v") + assert has_testbench_name(env, "aaa_tb.out") + assert has_testbench_name(env, "bbb/aaa_tb.v") + assert has_testbench_name(env, "bbb\\aaa_tb.v") + assert has_testbench_name(env, "aaa__tb.v") + assert has_testbench_name(env, "Aaa__Tb.v") + assert has_testbench_name(env, "bbb/aaa_tb.v") + assert has_testbench_name(env, "bbb\\aaa_tb.v") + + # -- Non testbench names. + assert not has_testbench_name(env, "aaatb.v") + assert not has_testbench_name(env, "aaa.v") + + +def test_is_verilog_src(): + """Tests the scons_util.is_verilog_src() function""" + + env: SConsEnvironment = DefaultEnvironment(ENV={}, tools=[]) + + # -- Verilog source names + assert is_verilog_src(env, "aaa.v") + assert is_verilog_src(env, "bbb/aaa.v") + assert is_verilog_src(env, "bbb\\aaa.v") + assert is_verilog_src(env, "aaatb.v") + assert is_verilog_src(env, "aaa_tb.v") + + # -- Non verilog source names. + assert not is_verilog_src(env, "aaatb.vv") + assert not is_verilog_src(env, "aaatb.V") + assert not is_verilog_src(env, "aaa_tb.vh") From 315fb3b8a8a8a352a2e10106357a366fed806e3b Mon Sep 17 00:00:00 2001 From: Zapta Date: Wed, 27 Nov 2024 07:35:12 -0800 Subject: [PATCH 3/9] Now using an independent scons env instead of the default one. The motivation is testibility of scons_utils.py. The default scons env is a singleton and thus inerferes with multiple tests in the same pytest process that need to create an env from scratch. Also added an assertion that the default env is not used by mistake in the SConstruct code. --- apio/scons/ecp5/SConstruct | 27 +++++++++++++-------------- apio/scons/gowin/SConstruct | 27 +++++++++++++-------------- apio/scons/ice40/SConstruct | 29 ++++++++++++++--------------- apio/scons/scons_util.py | 34 +++++++++++++++++++++++++++++++--- 4 files changed, 71 insertions(+), 46 deletions(-) diff --git a/apio/scons/ecp5/SConstruct b/apio/scons/ecp5/SConstruct index fb45b6bd..3217d4b5 100644 --- a/apio/scons/ecp5/SConstruct +++ b/apio/scons/ecp5/SConstruct @@ -41,7 +41,6 @@ import os from SCons.Script import ( Builder, - AlwaysBuild, GetOption, COMMAND_LINE_TARGETS, ARGUMENTS, @@ -189,11 +188,11 @@ bin_target = env.Bin(TARGET, pnr_target) build_target = env.Alias("build", bin_target) if VERBOSE_YOSYS: - AlwaysBuild(synth_target) + env.AlwaysBuild(synth_target) if VERBOSE_PNR: - AlwaysBuild(pnr_target) + env.AlwaysBuild(pnr_target) if VERBOSE_ALL: - AlwaysBuild(synth_target, pnr_target, build_target) + env.AlwaysBuild(synth_target, pnr_target, build_target) # -- Apio report. # -- Targets. @@ -202,7 +201,7 @@ report_action = get_report_action( env, SConstructId.SCONSTRUCT_ECP5, VERBOSE_PNR ) report_target = env.Alias("report", PNR_REPORT_FILE, report_action) -AlwaysBuild(report_target) +env.AlwaysBuild(report_target) # -- Apio upload. @@ -210,7 +209,7 @@ AlwaysBuild(report_target) # -- hardware.bit -> FPGA. programmer_cmd = get_programmer_cmd(env) upload_target = env.Alias("upload", bin_target, programmer_cmd) -AlwaysBuild(upload_target) +env.AlwaysBuild(upload_target) # -- Apio verify. @@ -300,7 +299,7 @@ env.Append(BUILDERS={"VCD": vcd_builder}) # -- Targets # -- (modules).v -> (modules).out verify_out_target = env.IVerilogVerify(TARGET, synth_srcs + test_srcs) -AlwaysBuild(verify_out_target) +env.AlwaysBuild(verify_out_target) verify_target = env.Alias("verify", verify_out_target) @@ -308,9 +307,9 @@ verify_target = env.Alias("verify", verify_out_target) # -- Targets. # -- (modules).v -> hardware.dot -> hardware.svg. dot_target = env.DOT(TARGET, synth_srcs) -AlwaysBuild(dot_target) +env.AlwaysBuild(dot_target) graphviz_target = env.GRAPHVIZ(TARGET, dot_target) -AlwaysBuild(graphviz_target) +env.AlwaysBuild(graphviz_target) graph_target = env.Alias("graph", graphviz_target) @@ -324,7 +323,7 @@ if "sim" in COMMAND_LINE_TARGETS: ) vcd_file_target = env.VCD(sim_out_target) waves_target = make_waves_target(env, vcd_file_target, sim_config) - AlwaysBuild(waves_target) + env.AlwaysBuild(waves_target) # -- Apio test. @@ -337,9 +336,9 @@ if "test" in COMMAND_LINE_TARGETS: test_out_target = env.IVerilogTestbench( sim_config.build_testbench_name, sim_config.srcs ) - AlwaysBuild(test_out_target) + env.AlwaysBuild(test_out_target) test_vcd_target = env.VCD(test_out_target) - AlwaysBuild(test_vcd_target) + env.AlwaysBuild(test_vcd_target) test_target = env.Alias( sim_config.build_testbench_name, [test_out_target, test_vcd_target] ) @@ -348,7 +347,7 @@ if "test" in COMMAND_LINE_TARGETS: # Create a target for the test command that depends on all the test # targets. tests_target = env.Alias("test", tests_targets) - AlwaysBuild(tests_target) + env.AlwaysBuild(tests_target) # -- Apio lint. @@ -395,7 +394,7 @@ lint_config_target = env.VerilatorConfig(TARGET, []) lint_out_target = env.Verilator(TARGET, synth_srcs + test_srcs) env.Depends(lint_out_target, lint_config_target) lint_target = env.Alias("lint", lint_out_target) -AlwaysBuild(lint_target) +env.AlwaysBuild(lint_target) # -- Handle the cleanu of the artifact files. diff --git a/apio/scons/gowin/SConstruct b/apio/scons/gowin/SConstruct index b31ef661..298b27f6 100644 --- a/apio/scons/gowin/SConstruct +++ b/apio/scons/gowin/SConstruct @@ -41,7 +41,6 @@ import os from SCons.Script import ( Builder, - AlwaysBuild, GetOption, COMMAND_LINE_TARGETS, ARGUMENTS, @@ -181,11 +180,11 @@ bin_target = env.Bin(TARGET, pnr_target) build_target = env.Alias("build", bin_target) if VERBOSE_YOSYS: - AlwaysBuild(synth_target) + env.AlwaysBuild(synth_target) if VERBOSE_PNR: - AlwaysBuild(pnr_target) + env.AlwaysBuild(pnr_target) if VERBOSE_ALL: - AlwaysBuild(synth_target, pnr_target, build_target) + env.AlwaysBuild(synth_target, pnr_target, build_target) # -- Apio report. # -- Targets. @@ -194,7 +193,7 @@ report_action = get_report_action( env, SConstructId.SCONSTRUCT_GOWIN, VERBOSE_PNR ) report_target = env.Alias("report", PNR_REPORT_FILE, report_action) -AlwaysBuild(report_target) +env.AlwaysBuild(report_target) # -- Apio upload. @@ -202,7 +201,7 @@ AlwaysBuild(report_target) # -- hardware.fs -> FPGA. programmer_cmd = get_programmer_cmd(env) upload_target = env.Alias("upload", bin_target, programmer_cmd) -AlwaysBuild(upload_target) +env.AlwaysBuild(upload_target) # -- Apio verify. @@ -293,7 +292,7 @@ env.Append(BUILDERS={"VCD": vcd_builder}) # -- Targets # -- (modules).v -> (modules).out verify_out_target = env.IVerilogVerify(TARGET, synth_srcs + test_srcs) -AlwaysBuild(verify_out_target) +env.AlwaysBuild(verify_out_target) verify_target = env.Alias("verify", verify_out_target) @@ -301,9 +300,9 @@ verify_target = env.Alias("verify", verify_out_target) # -- Targets. # -- (modules).v -> hardware.dot -> hardware.svg. dot_target = env.DOT(TARGET, synth_srcs) -AlwaysBuild(dot_target) +env.AlwaysBuild(dot_target) graphviz_target = env.GRAPHVIZ(TARGET, dot_target) -AlwaysBuild(graphviz_target) +env.AlwaysBuild(graphviz_target) graph_target = env.Alias("graph", graphviz_target) @@ -317,7 +316,7 @@ if "sim" in COMMAND_LINE_TARGETS: ) vcd_file_target = env.VCD(sim_out_target) waves_target = make_waves_target(env, vcd_file_target, sim_config) - AlwaysBuild(waves_target) + env.AlwaysBuild(waves_target) # -- Apio test. @@ -330,9 +329,9 @@ if "test" in COMMAND_LINE_TARGETS: test_out_target = env.IVerilogTestbench( sim_config.build_testbench_name, sim_config.srcs ) - AlwaysBuild(test_out_target) + env.AlwaysBuild(test_out_target) test_vcd_target = env.VCD(test_out_target) - AlwaysBuild(test_vcd_target) + env.AlwaysBuild(test_vcd_target) test_target = env.Alias( sim_config.build_testbench_name, [test_out_target, test_vcd_target] ) @@ -341,7 +340,7 @@ if "test" in COMMAND_LINE_TARGETS: # Create a target for the test command that depends on all the test # targets. tests_target = env.Alias("test", tests_targets) - AlwaysBuild(tests_target) + env.AlwaysBuild(tests_target) # -- Apio lint. @@ -384,7 +383,7 @@ lint_config_target = env.VerilatorConfig(TARGET, []) lint_out_target = env.Verilator(TARGET, synth_srcs + test_srcs) env.Depends(lint_out_target, lint_config_target) lint_target = env.Alias("lint", lint_out_target) -AlwaysBuild(lint_target) +env.AlwaysBuild(lint_target) # -- Handle the cleanu of the artifact files. diff --git a/apio/scons/ice40/SConstruct b/apio/scons/ice40/SConstruct index 2d54f328..b1bbc4ba 100644 --- a/apio/scons/ice40/SConstruct +++ b/apio/scons/ice40/SConstruct @@ -41,7 +41,6 @@ import os from SCons.Script import ( Builder, - AlwaysBuild, GetOption, COMMAND_LINE_TARGETS, ARGUMENTS, @@ -196,11 +195,11 @@ bin_target = env.Bin(TARGET, pnr_target) build_target = env.Alias("build", bin_target) if VERBOSE_YOSYS: - AlwaysBuild(synth_target) + env.AlwaysBuild(synth_target) if VERBOSE_PNR: - AlwaysBuild(pnr_target) + env.AlwaysBuild(pnr_target) if VERBOSE_ALL: - AlwaysBuild(synth_target, pnr_target, build_target) + env.AlwaysBuild(synth_target, pnr_target, build_target) # -- Apio report. # -- Targets. @@ -210,7 +209,7 @@ report_action = get_report_action( env, SConstructId.SCONSTRUCT_ICE40, VERBOSE_PNR ) report_target = env.Alias("report", PNR_REPORT_FILE, report_action) -AlwaysBuild(report_target) +env.AlwaysBuild(report_target) # -- Apio upload. @@ -218,14 +217,14 @@ AlwaysBuild(report_target) # -- hardware.bin -> FPGA. programmer_cmd: str = get_programmer_cmd(env) upload_target = env.Alias("upload", bin_target, programmer_cmd) -AlwaysBuild(upload_target) +env.AlwaysBuild(upload_target) # -- Apio time. # -- Targets. # -- hardware.asc -> hardware.rpt time_rpt_target = env.Time(pnr_target) -AlwaysBuild(time_rpt_target) +env.AlwaysBuild(time_rpt_target) time_target = env.Alias("time", time_rpt_target) @@ -318,7 +317,7 @@ env.Append(BUILDERS={"VCD": vcd_builder}) # -- Targets # -- (modules).v -> (modules).out verify_out_target = env.IVerilogVerify(TARGET, synth_srcs + test_srcs) -AlwaysBuild(verify_out_target) +env.AlwaysBuild(verify_out_target) verify_target = env.Alias("verify", verify_out_target) @@ -326,9 +325,9 @@ verify_target = env.Alias("verify", verify_out_target) # -- Targets. # -- (modules).v -> hardware.dot -> hardware.svg. dot_target = env.DOT(TARGET, synth_srcs) -AlwaysBuild(dot_target) +env.AlwaysBuild(dot_target) graphviz_target = env.GRAPHVIZ(TARGET, dot_target) -AlwaysBuild(graphviz_target) +env.AlwaysBuild(graphviz_target) graph_target = env.Alias("graph", graphviz_target) @@ -342,7 +341,7 @@ if "sim" in COMMAND_LINE_TARGETS: ) vcd_file_target = env.VCD(sim_out_target) waves_target = make_waves_target(env, vcd_file_target, sim_config) - AlwaysBuild(waves_target) + env.AlwaysBuild(waves_target) # -- Apio test. @@ -355,9 +354,9 @@ if "test" in COMMAND_LINE_TARGETS: test_out_target = env.IVerilogTestbench( sim_config.build_testbench_name, sim_config.srcs ) - AlwaysBuild(test_out_target) + env.AlwaysBuild(test_out_target) test_vcd_target = env.VCD(test_out_target) - AlwaysBuild(test_vcd_target) + env.AlwaysBuild(test_vcd_target) test_target = env.Alias( sim_config.build_testbench_name, [test_out_target, test_vcd_target] ) @@ -366,7 +365,7 @@ if "test" in COMMAND_LINE_TARGETS: # Create a target for the test command that depends on all the test # targets. tests_target = env.Alias("test", tests_targets) - AlwaysBuild(tests_target) + env.AlwaysBuild(tests_target) # -- Apio lint. @@ -410,7 +409,7 @@ lint_config_target = env.VerilatorConfig(TARGET, []) lint_out_target = env.Verilator(TARGET, synth_srcs + test_srcs) env.Depends(lint_out_target, lint_config_target) lint_target = env.Alias("lint", lint_out_target) -AlwaysBuild(lint_target) +env.AlwaysBuild(lint_target) # -- Handle the cleanu of the artifact files. diff --git a/apio/scons/scons_util.py b/apio/scons/scons_util.py index f063b96b..fd8d954c 100644 --- a/apio/scons/scons_util.py +++ b/apio/scons/scons_util.py @@ -28,10 +28,12 @@ from SCons.Node import NodeList from SCons.Node.FS import File from SCons.Node.Alias import Alias -from SCons.Script import DefaultEnvironment +from SCons.Script import Environment from SCons.Script.SConscript import SConsEnvironment from SCons.Action import FunctionAction, Action from SCons.Builder import Builder +import SCons.Defaults + # -- All the build files and other artifcats are created in this this # -- subdirectory. @@ -109,8 +111,15 @@ def is_windows(env: SConsEnvironment) -> bool: def create_construction_env(args: Dict[str, str]) -> SConsEnvironment: """Creates a scons env. Should be called very early in SConstruct.py""" - # Create the default env. - env: SConsEnvironment = DefaultEnvironment(ENV=os.environ, tools=[]) + + # Check that the default env is not used by mistake. SConstruct to use + # only our env. + check_default_scons_env_not_used() + + # Create the env. We don't use the DefaultEnvironment (a singleton) to + # allow the creation in pytest multiple test environments in the same + # tesing process. + env: SConsEnvironment = Environment(ENV=os.environ, tools=[]) # Add the args dictionary as a new ARGUMENTS var. assert env.get("ARGUMENTS") is None @@ -820,3 +829,22 @@ def set_up_cleanup(env: SConsEnvironment) -> None: # -- Associate all the files with the dummy target. for file in files_to_clean: env.Clean(dummy_target, str(file)) + + # -- Since the this function is called at the end of the SCOnstruct files, + # -- we use it to check that the SConstruct code didn't use by mistate + # -- the scons default env instead of the env we created in this file. + check_default_scons_env_not_used() + + +def check_default_scons_env_not_used(): + """A method to check that the default scons env has not been used. + We avoid using the default scons env because it's a singleton and thus, + don't play well with pytest where multiple indendepent scons envs + are created by tests in the same python process. This function asserts + that the default scons env was not instantiated by mistake, e.g. + by calling in SConstrut AllwaysBuild() instead of env.AlwaysBuild(). + """ + + # pylint: disable=protected-access + assert SCons.Defaults._default_env is None + # pylint: enable=protected-access From 3de3ad5faecaac3999439806e44533b3249c45c9 Mon Sep 17 00:00:00 2001 From: Zapta Date: Wed, 27 Nov 2024 08:54:50 -0800 Subject: [PATCH 4/9] Added tests for scons_util.py --- test/test_scons_util.py | 122 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 118 insertions(+), 4 deletions(-) diff --git a/test/test_scons_util.py b/test/test_scons_util.py index 5271ea61..56583a93 100644 --- a/test/test_scons_util.py +++ b/test/test_scons_util.py @@ -2,14 +2,24 @@ Tests of scons_util.py """ +from typing import Dict from test.conftest import ApioRunner -from SCons.Script import DefaultEnvironment +import pytest from SCons.Script.SConscript import SConsEnvironment from SCons.Node.FS import FS, File +from SCons.Defaults import DefaultEnvironment +import SCons.Defaults from apio.scons.scons_util import ( make_verilog_src_scanner, has_testbench_name, is_verilog_src, + create_construction_env, + get_args, + arg_str, + arg_bool, + is_windows, + force_colors, + check_default_scons_env_not_used, ) DEPENDECIES_TEST_TEXT = """ @@ -21,6 +31,28 @@ """ +def _make_test_env(args: Dict[str, str] = None) -> SConsEnvironment: + """Creates a fresh apio scons env with given args.""" + + # -- Check that the default scons env was not used. + check_default_scons_env_not_used() + + # -- Default, when we don't really care about the content. + if args is None: + args = { + "platform_id": "darwin_arm64", + "force_colors": "False", + } + + # -- Use the apio specific env creation function. + env = create_construction_env(args) + + # -- Check that the default scons env was not used. + check_default_scons_env_not_used() + + return env + + def test_dependencies(apio_runner: ApioRunner): """Test the verilog scanner which scans a verilog file and extract reference of files it uses. @@ -33,7 +65,7 @@ def test_dependencies(apio_runner: ApioRunner): f.write(DEPENDECIES_TEST_TEXT) # -- Create a scanner - env: SConsEnvironment = DefaultEnvironment(ENV={}, tools=[]) + env = _make_test_env() scanner = make_verilog_src_scanner(env) # -- Run the scanner. It returns a list of File. @@ -54,7 +86,7 @@ def test_dependencies(apio_runner: ApioRunner): def test_has_testbench_name(): """Tests the scons_util.test_is_testbench() function""" - env: SConsEnvironment = DefaultEnvironment(ENV={}, tools=[]) + env = _make_test_env # -- Testbench names assert has_testbench_name(env, "aaa_tb.v") @@ -74,7 +106,7 @@ def test_has_testbench_name(): def test_is_verilog_src(): """Tests the scons_util.is_verilog_src() function""" - env: SConsEnvironment = DefaultEnvironment(ENV={}, tools=[]) + env = _make_test_env() # -- Verilog source names assert is_verilog_src(env, "aaa.v") @@ -87,3 +119,85 @@ def test_is_verilog_src(): assert not is_verilog_src(env, "aaatb.vv") assert not is_verilog_src(env, "aaatb.V") assert not is_verilog_src(env, "aaa_tb.vh") + + +def test_env_args(): + """Tests the scons env args retrieval.""" + + args = { + "platform_id": "my_platform", + "AAA": "my_str", + "BBB": "False", + "CCC": "True", + } + + env = _make_test_env(args.copy()) + result_args = get_args(env) + assert result_args == args + + # -- String args + assert arg_str(env, "AAA", "") == "my_str" + assert arg_str(env, "ZZZ", "") == "" + assert arg_str(env, "ZZZ", "abc") == "abc" + assert arg_str(env, "ZZZ", None) is None + + # -- Bool args + assert not arg_bool(env, "BBB", None) + assert arg_bool(env, "CCC", None) + assert not arg_bool(env, "ZZZ", False) + assert arg_bool(env, "ZZZ", True) + assert arg_bool(env, "ZZZ", None) is None + + +def test_env_platform_id(): + """Tests the env handling of the paltform_id var.""" + + # -- Test with a non windows platform id. + env = _make_test_env({"platform_id": "darwin_arm64"}) + assert not is_windows(env) + + # -- Test with a windows platform id. + env = _make_test_env({"platform_id": "windows_amd64"}) + assert is_windows(env) + + +def test_env_forcing_color(): + """Tests the color forcing functionality of the scons env.""" + + # -- Color forcing turned on (apio writes to a terminal) + env = _make_test_env( + {"force_colors": "True", "platform_id": "darwin_arm64"} + ) + assert force_colors(env) + + # -- Color forcing turned off (apio process is pipped out) + env = _make_test_env( + {"force_colors": "False", "platform_id": "darwin_arm64"} + ) + assert not force_colors(env) + + +def test_default_env_check(): + """Teest that check_default_scons_env_not_used() throws an assertion + exception when the default scons env is used. + """ + # -- Since _default_env is private we supress lint warning. + # pylint: disable=protected-access + + # -- Should not throw an exception when _default_env is not None. + assert SCons.Defaults._default_env is None + check_default_scons_env_not_used() + + # -- Using the default enrivornment makes _default_env non None. + DefaultEnvironment(ENV={}, tools=[]) + assert SCons.Defaults._default_env is not None + + # -- This checks that the function has a failing assertion. + with pytest.raises(AssertionError): + check_default_scons_env_not_used() + + # -- Restore _default_env to None, for any other test that may sill need + # -- to run. + SCons.Defaults._default_env = None + + # pylint: enable=protected-access From ff64e5bc7627a14f79e2d55a8e0d6ec252d29e7b Mon Sep 17 00:00:00 2001 From: Zapta Date: Wed, 27 Nov 2024 08:57:54 -0800 Subject: [PATCH 5/9] Added aliases and shortcuts to the make file targets. Existing target names stated with no change. --- DEVELOPERS.md | 4 ++-- Makefile | 14 +++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/DEVELOPERS.md b/DEVELOPERS.md index a0a0dd6a..6347a77e 100644 --- a/DEVELOPERS.md +++ b/DEVELOPERS.md @@ -13,7 +13,7 @@ make check For complete tests with several python versions run the command below. ```shell -make check_all +make check-all ``` For quick tests that that don't load lengthy packagtes from the internet @@ -38,7 +38,7 @@ When running any of the commands below, a test coverage is generated in the ``` make test // Partial coverage by offline tests make check // Full coverage -make check_all // Full coverage by the last python env run. +make check-all // Full coverage by the last python env run. ``` diff --git a/Makefile b/Makefile index 2f208e20..2e6f3510 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,9 @@ .PHONY: deps cenv env lint test presubmit publish_test publish install +# NOTE: Some targets have a shortcuts or alias names that are listed on the same line. +# The are provided for convinience or for backward competibility. For example +# 'check-all' has the aliases 'check_all' and 'ca'. + # Install dependencies for apio development deps: python -m pip install --upgrade pip @@ -18,31 +22,31 @@ env: # Lint only, no tests. -lint: +lint l: python -m tox -e lint # Offline tests only, no lint, single python version, skipping online tests. # This is a partial but fast test. -test: +test t: python -m tox --skip-missing-interpreters false -e py312 -- --offline # Tests and lint, single python version, all tests including online.. # This is a thorough but slow test and sufficient for testign before # commiting changes run this before submitting code. - check: +check c: python -m tox --skip-missing-interpreters false -e lint,py312 # Tests and lint, multiple python versions. # Should be be run automatically on github. -check_all: +check-all check_all ca: python -m tox --skip-missing-interpreters false # Publish to testPypi -publish_test: +publish-test publish_test: flit publish --repository testpypi From d59a31aa81faf8720d0c5569b613b01b3f6a3031 Mon Sep 17 00:00:00 2001 From: Zapta Date: Thu, 28 Nov 2024 14:28:16 -0800 Subject: [PATCH 6/9] Updated the comments of Makefile. --- Makefile | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 2e6f3510..202d9caf 100644 --- a/Makefile +++ b/Makefile @@ -5,29 +5,44 @@ # 'check-all' has the aliases 'check_all' and 'ca'. # Install dependencies for apio development +# +# Usage: +# make deps deps: python -m pip install --upgrade pip pip install flit black flake8 pylint tox pytest semantic-version pyserial importlib-metadata # Create the virtual-environment and update dependencies +# +# Usage: +# make cenv cenv: python3 -m venv venv python3 -m venv venv --upgrade - +# Usage +# make env env: @echo "For entering the virtual-environment just type:" @echo ". venv/bin/activate" # Lint only, no tests. +# +# Usage: +# make lint +# make l lint l: python -m tox -e lint # Offline tests only, no lint, single python version, skipping online tests. # This is a partial but fast test. +# +# Usage: +# make test +# make t test t: python -m tox --skip-missing-interpreters false -e py312 -- --offline @@ -35,27 +50,45 @@ test t: # Tests and lint, single python version, all tests including online.. # This is a thorough but slow test and sufficient for testign before # commiting changes run this before submitting code. +# +# Usage: +# make check +# make c check c: python -m tox --skip-missing-interpreters false -e lint,py312 # Tests and lint, multiple python versions. # Should be be run automatically on github. +# +# Usage: +# make check-all +# make check_all // deprecated, to be deleted. +# make ca check-all check_all ca: python -m tox --skip-missing-interpreters false # Publish to testPypi +# +# Usage: +# make publish-test +# make publish_test // deprecated, to be deleted. publish-test publish_test: flit publish --repository testpypi # Publish to PyPi +# +# Usage: +# make publish publish: python -m flit publish ## Install the tool locally +# +# Usage: +# make instll install: flit build flit install - From 07412c80d203467de5c387a5cce2b5cf899cdb46 Mon Sep 17 00:00:00 2001 From: Zapta Date: Thu, 28 Nov 2024 14:48:22 -0800 Subject: [PATCH 7/9] Added more tests of scons_util.py --- apio/scons/scons_util.py | 38 +++---- test/test_scons_util.py | 225 +++++++++++++++++++++++++++++++++++---- 2 files changed, 223 insertions(+), 40 deletions(-) diff --git a/apio/scons/scons_util.py b/apio/scons/scons_util.py index fd8d954c..a17311d3 100644 --- a/apio/scons/scons_util.py +++ b/apio/scons/scons_util.py @@ -62,11 +62,13 @@ def map_params( ) -> str: """A common function construct a command string snippet from a list of arguments. The functon does the following: - 1. If non replace with [] - 2. Drops empty items. + 1. If params arg is None replace it with [] + 2. Drops empty or white space only items. 3. Maps the items using the format string which contains exactly one placeholder {}. 4. Joins the items with a white space char. + + For examples, see the unit test at test_scons_util.py. """ # None designates empty list. Avoiding the pylint non safe default warning. if params is None: @@ -128,12 +130,9 @@ def create_construction_env(args: Dict[str, str]) -> SConsEnvironment: # Evaluate the optional force_color arg and set its value # an env var on its own. assert env.get("FORCE_COLORS") is None - env.Replace(FORCE_COLORS=False) # Tentative. + env.Replace(FORCE_COLORS=False) # Tentative, so we can call arg_bool. flag = arg_bool(env, "force_colors", False) - env.Replace(FORCE_COLORS=flag) # Tentative. - if not flag: - warning(env, "Not forcing scons text colors.") - + env.Replace(FORCE_COLORS=flag) # Set the IS_WINDOWS flag based on the required "platform_id" arg. platform_id = arg_str(env, "platform_id", False) # Note: this is a programming error, not a user error. @@ -758,7 +757,7 @@ def print_pnr_report( # Enable for debugging a scons process and call from SConstruct. # -# def wait_for_remote_debugger(env: SConsEnvironment): +# def wait_for_remote_debugger(): # """For developement only. Useful for debugging SConstruct scripts that # apio runs as a subprocesses. Call this from the SCconstruct script, run # apio from a command line, and then connect with the Visual Studio Code @@ -771,15 +770,13 @@ def print_pnr_report( # # -- 5678 is the default debugger port. # port = 5678 -# msg( -# env, -# f"Waiting for remote debugger on port localhost:{port}.", -# fg="magenta", +# print( +# f"Waiting for remote debugger on port localhost:{port}." # ) # debugpy.listen(port) -# msg(env, "Attach with the Visual Studio Code debugger.") +# print("Attach with the Visual Studio Code debugger.") # debugpy.wait_for_client() -# msg(env, "Remote debugger is attached.", fg="green") +# print("Remote debugger is attached.") def set_up_cleanup(env: SConsEnvironment) -> None: @@ -789,7 +786,7 @@ def set_up_cleanup(env: SConsEnvironment) -> None: # -- Should be called only when the 'clean' target is specified. # -- If this fails, this is a programming error and not a user error. - assert env.GetOption("clean") + assert env.GetOption("clean"), "Option 'clean' is missing." # -- Get the list of all files to clean. Scons adds to the list non # -- existing files from other targets it encountered. @@ -822,13 +819,10 @@ def set_up_cleanup(env: SConsEnvironment) -> None: files_to_clean.extend(legacy_files_to_clean) # -- Create a dummy target. I - dummy_target = env.Command( - "no-such-file-1", "no-such-file-2", "no-such-action" - ) + dummy_target = env.Command("cleanup-target", "", "") # -- Associate all the files with the dummy target. - for file in files_to_clean: - env.Clean(dummy_target, str(file)) + env.Clean(dummy_target, files_to_clean) # -- Since the this function is called at the end of the SCOnstruct files, # -- we use it to check that the SConstruct code didn't use by mistate @@ -846,5 +840,7 @@ def check_default_scons_env_not_used(): """ # pylint: disable=protected-access - assert SCons.Defaults._default_env is None + assert ( + SCons.Defaults._default_env is None + ), "Detected usage of scons default env." # pylint: enable=protected-access diff --git a/test/test_scons_util.py b/test/test_scons_util.py index 56583a93..87e48ec1 100644 --- a/test/test_scons_util.py +++ b/test/test_scons_util.py @@ -2,13 +2,18 @@ Tests of scons_util.py """ +from pathlib import Path from typing import Dict from test.conftest import ApioRunner import pytest from SCons.Script.SConscript import SConsEnvironment +import SCons.Script.SConsOptions from SCons.Node.FS import FS, File -from SCons.Defaults import DefaultEnvironment +import SCons.Node.FS +import SCons.Environment import SCons.Defaults +import SCons.Script.Main +from SCons.Script import SetOption from apio.scons.scons_util import ( make_verilog_src_scanner, has_testbench_name, @@ -20,6 +25,13 @@ is_windows, force_colors, check_default_scons_env_not_used, + set_up_cleanup, + map_params, + fatal_error, + error, + warning, + info, + msg, ) DEPENDECIES_TEST_TEXT = """ @@ -31,11 +43,77 @@ """ -def _make_test_env(args: Dict[str, str] = None) -> SConsEnvironment: - """Creates a fresh apio scons env with given args.""" +class SconsHacks: + """A collection of staticmethods that encapsulate scons access outside of + the official scons API. Hopefully this will not be too difficult to adapt + in future versions of SCons.""" + + @staticmethod + def reset_scons_state() -> None: + """Reset the relevant SCons global variables. וUnfurtunally scons + uses a few global variables to hold its state. This works well in + normal operation where an scons process contains a single scons + session but with pytest testing, where multiple independent tests + are running in the same process, we need to reset though variables + before each test.""" + + # -- We don't reset the _default_env, just make sure it was never used. + assert not SconsHacks.default_scons_env_exists() + + # -- The Cons.Script.Main.OptionsParser variables contains the command + # -- line options of scons. We reset them here and tests can access + # -- them using SetOption() and GetOption(). + + parser = SCons.Script.SConsOptions.Parser("my_fake_version") + values = SCons.Script.SConsOptions.SConsValues( + parser.get_default_values() + ) + parser.parse_args(args=[], values=values) + SCons.Script.Main.OptionsParser = parser + + # -- Reset the scons target list variable. + SCons.Node.FS.default_fs = None + + # -- Clear the SCons targets + SCons.Environment.CleanTargets = {} + + # -- Check again, just in case. + assert not SconsHacks.default_scons_env_exists() + + @staticmethod + def get_targets() -> Dict: + """Get the scons {target -> dependencies} dictionary.""" + return SCons.Environment.CleanTargets + + @staticmethod + def default_scons_env_exists() -> bool: + """Tests if the default scons env was created. The default env is a + singleton that don't play well with pytest so we avoid it and use + instead an expclicit environment we create, and verify that the + default environement was not used by mistake, for example by + SConstruct calling AllwaysBuild() instead of env.AllwaysBuild(). + """ + # pylint: disable=protected-access + return SCons.Defaults._default_env is not None + # pylint: enable=protected-access + + @staticmethod + def clear_scons_default_env() -> None: + """Clear the scons default environment.""" + # pylint: disable=protected-access + SCons.Defaults._default_env = None + # pylint: enable=protected-access + + +def _make_test_env( + args: Dict[str, str] = None, extra_args: Dict[str, str] = None +) -> SConsEnvironment: + """Creates a fresh apio scons env with given args. The env is created + with a reference to the current directory. + """ - # -- Check that the default scons env was not used. - check_default_scons_env_not_used() + # -- Bring scons to a starting state. + SconsHacks.reset_scons_state() # -- Default, when we don't really care about the content. if args is None: @@ -44,7 +122,11 @@ def _make_test_env(args: Dict[str, str] = None) -> SConsEnvironment: "force_colors": "False", } - # -- Use the apio specific env creation function. + # -- If specified, overite/add extra args. + if extra_args: + args.update(extra_args) + + # -- Use the apio's scons env creation function. env = create_construction_env(args) # -- Check that the default scons env was not used. @@ -181,23 +263,128 @@ def test_default_env_check(): """Teest that check_default_scons_env_not_used() throws an assertion exception when the default scons env is used. """ - # -- Since _default_env is private we supress lint warning. - # pylint: disable=protected-access + # -- The starting point is having no default env. + assert not SconsHacks.default_scons_env_exists() - # -- Should not throw an exception when _default_env is not None. - assert SCons.Defaults._default_env is None + # -- Should not throw an exception since the default env does not exist. check_default_scons_env_not_used() - # -- Using the default enrivornment makes _default_env non None. - DefaultEnvironment(ENV={}, tools=[]) - assert SCons.Defaults._default_env is not None + # -- Create the scons default env. + SCons.Defaults.DefaultEnvironment(ENV={}, tools=[]) + assert SconsHacks.default_scons_env_exists() - # -- This checks that the function has a failing assertion. - with pytest.raises(AssertionError): + # -- Verify that the assertion in the function fails. + with pytest.raises(AssertionError) as e: check_default_scons_env_not_used() - # -- Restore _default_env to None, for any other test that may sill need - # -- to run. - SCons.Defaults._default_env = None + # -- Verify the assertion text. + assert "Detected usage of scons default env" in str(e.value) + + # -- Restore the no-default-env state for the reset of the tests. + SconsHacks.clear_scons_default_env() + + +def test_set_up_cleanup_ok(apio_runner: ApioRunner): + """Tests the success path of set_up_cleanup().""" + + with apio_runner.in_disposable_temp_dir(): + + # -- Create an env with 'clean' option set. + env = _make_test_env() + SetOption("clean", True) + + # -- Create files that shouldn't be cleaned up. + Path("my_source.v") + Path("apio.ini") + + # -- Create files that should be cleaned up. + Path("zadig.ini").touch() + Path("_build").mkdir() + Path("_build/aaa").touch() + Path("_build/bbb").touch() + + # -- Run the cleanup setup. It's expected to add a single + # -- target with the dependencies to clean up. + assert len(SconsHacks.get_targets()) == 0 + set_up_cleanup(env) + assert len(SconsHacks.get_targets()) == 1 + + # -- Get the target and its dependencies + items_list = list(SconsHacks.get_targets().items()) + target, dependencies = items_list[0] + + # -- Verify the tartget name, hard coded in set_up_cleanup() + assert target.name == "cleanup-target" + + # -- Verif the dependencies. These are the files to delete. + file_names = [x.name for x in dependencies] + assert file_names == ["aaa", "bbb", "zadig.ini", "_build"] + + +def test_set_up_cleanup_errors(): + """Tests the error conditions of the set_up_cleanup() function.""" + + env = _make_test_env() + + # -- Try without the option 'clean'. It should fail. + with pytest.raises(AssertionError) as e: + set_up_cleanup(env) + assert "Option 'clean' is missing" in str(e) + + # -- Try with the option 'clean'=false. It should fail. + SetOption("clean", False) + with pytest.raises(AssertionError) as e: + set_up_cleanup(env) + assert "Option 'clean' is missing" in str(e) + + +def test_map_params(): + """Test the map_params() function.""" + + env = _make_test_env() + + # -- Empty cases + assert map_params(env, [], "x_{}_y") == "" + assert map_params(env, ["", " "], "x_{}_y") == "" + + # -- Non empty cases + assert map_params(env, ["a"], "x_{}_y") == "x_a_y" + assert map_params(env, [" a "], "x_{}_y") == "x_a_y" + assert map_params(env, ["a", "a", "b"], "x_{}_y") == "x_a_y x_a_y x_b_y" + + +def test_log_functions(capsys): + """Tests the fatal_error() function.""" + + # -- Create the scons env. + env = _make_test_env() + + # -- Test msg() + msg(env, "My msg") + captured = capsys.readouterr() + assert "My msg\n" == captured.out + + # -- Test info() + info(env, "My info") + captured = capsys.readouterr() + assert "Info: My info\n" == captured.out + + # -- Test warning() + warning(env, "My warning") + captured = capsys.readouterr() + assert "Warning: My warning\n" == captured.out + + # -- Test error() + error(env, "My error") + captured = capsys.readouterr() + assert "Error: My error\n" == captured.out + + # -- Test fatal_error() + with pytest.raises(SystemExit) as e: + fatal_error(env, "My fatal error") + assert e.value.code == 1 + captured = capsys.readouterr() + assert "Error: My fatal error\n" == captured.out + - # pylint: enable=protected-access +# Add test for color forcing on/off From 92e9d971a3a070e72aa84df353b94e713355f913 Mon Sep 17 00:00:00 2001 From: Zapta Date: Thu, 28 Nov 2024 20:08:39 -0800 Subject: [PATCH 8/9] Added more scons_util.py tests. --- .vscode/launch.json | 2 +- apio/scons/ecp5/SConstruct | 6 +- apio/scons/gowin/SConstruct | 6 +- apio/scons/ice40/SConstruct | 6 +- apio/scons/scons_util.py | 27 +----- test/test_scons_util.py | 158 +++++++++++++++++++++++++++++++++++- 6 files changed, 166 insertions(+), 39 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index d317f68b..3680f650 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -300,7 +300,7 @@ "module": "pytest", "args": [ "-s", - "test/test_scons_util.py" + "test/test_scons_util.py::test_make_verilator_config_builder" ], "console": "integratedTerminal", "justMyCode": false, diff --git a/apio/scons/ecp5/SConstruct b/apio/scons/ecp5/SConstruct index 3217d4b5..6e37a6ef 100644 --- a/apio/scons/ecp5/SConstruct +++ b/apio/scons/ecp5/SConstruct @@ -71,13 +71,13 @@ from apio.scons.scons_util import ( set_up_cleanup, ) +# # -- Uncomment for debugging of the scons subprocess using a remote debugger. +# from apio.scons import scons_util +# scons_util.wait_for_remote_debugger() # -- Create the environment env = create_construction_env(ARGUMENTS) -# -- Uncomment for debugging of the scons subprocess using a remote debugger. -# from apio.scons import scons_util -# scons_util.wait_for_remote_debugger(env) # -- Get arguments. FPGA_SIZE = arg_str(env, "fpga_size", "") diff --git a/apio/scons/gowin/SConstruct b/apio/scons/gowin/SConstruct index 298b27f6..0e909ce9 100644 --- a/apio/scons/gowin/SConstruct +++ b/apio/scons/gowin/SConstruct @@ -70,13 +70,13 @@ from apio.scons.scons_util import ( set_up_cleanup, ) +# # -- Uncomment for debugging of the scons subprocess using a remote debugger. +# from apio.scons import scons_util +# scons_util.wait_for_remote_debugger() # -- Create the environment env = create_construction_env(ARGUMENTS) -# -- Uncomment for debugging of the scons subprocess using a remote debugger. -# from apio.scons import scons_util -# scons_util.wait_for_remote_debugger(env) # -- Get arguments. FPGA_MODEL = arg_str(env, "fpga_model", "") diff --git a/apio/scons/ice40/SConstruct b/apio/scons/ice40/SConstruct index b1bbc4ba..bc4bb616 100644 --- a/apio/scons/ice40/SConstruct +++ b/apio/scons/ice40/SConstruct @@ -70,13 +70,13 @@ from apio.scons.scons_util import ( set_up_cleanup, ) +# # -- Uncomment for debugging of the scons subprocess using a remote debugger. +# from apio.scons import scons_util +# scons_util.wait_for_remote_debugger() # -- Create the environment env = create_construction_env(ARGUMENTS) -# -- Uncomment for debugging of the scons subprocess using a remote debugger. -# from apio.scons import scons_util -# scons_util.wait_for_remote_debugger(env) # -- Get arguments. FPGA_SIZE = arg_str(env, "fpga_size", "") diff --git a/apio/scons/scons_util.py b/apio/scons/scons_util.py index a17311d3..11157979 100644 --- a/apio/scons/scons_util.py +++ b/apio/scons/scons_util.py @@ -279,29 +279,6 @@ def dump_env_vars(env: SConsEnvironment) -> None: print("----- Env vars end -------") -def get_verilator_warning_params(env: SConsEnvironment) -> str: - """Construct from the nowwarn and warn arguments an option list - for verilator. These values are specified by the user to the - apio lint param. - - To test: apio lint --warn aaa,bbb --nowarn ccc,ddd - """ - - no_warn_list = arg_str(env, "nowarn", "").split(",") - warn_list = arg_str(env, "warn", "").split(",") - # No warn. - result = "" - for warn in no_warn_list: - if warn != "": - result += " -Wno-" + warn - # Warn. - for warn in warn_list: - if warn != "": - result += " -Wwarn-" + warn - - return result - - def get_programmer_cmd(env: SConsEnvironment) -> str: """Return the programmer command as derived from the scons "prog" arg.""" @@ -770,9 +747,7 @@ def print_pnr_report( # # -- 5678 is the default debugger port. # port = 5678 -# print( -# f"Waiting for remote debugger on port localhost:{port}." -# ) +# print(f"Waiting for remote debugger on port localhost:{port}.") # debugpy.listen(port) # print("Attach with the Visual Studio Code debugger.") # debugpy.wait_for_client() diff --git a/test/test_scons_util.py b/test/test_scons_util.py index 87e48ec1..3d4c3a2e 100644 --- a/test/test_scons_util.py +++ b/test/test_scons_util.py @@ -2,6 +2,7 @@ Tests of scons_util.py """ +from os.path import isfile, exists from pathlib import Path from typing import Dict from test.conftest import ApioRunner @@ -14,6 +15,7 @@ import SCons.Defaults import SCons.Script.Main from SCons.Script import SetOption +from pytest import LogCaptureFixture from apio.scons.scons_util import ( make_verilog_src_scanner, has_testbench_name, @@ -32,6 +34,10 @@ warning, info, msg, + get_constraint_file, + get_programmer_cmd, + make_verilator_config_builder, + get_source_files, ) DEPENDECIES_TEST_TEXT = """ @@ -119,7 +125,6 @@ def _make_test_env( if args is None: args = { "platform_id": "darwin_arm64", - "force_colors": "False", } # -- If specified, overite/add extra args. @@ -353,7 +358,7 @@ def test_map_params(): assert map_params(env, ["a", "a", "b"], "x_{}_y") == "x_a_y x_a_y x_b_y" -def test_log_functions(capsys): +def test_log_functions(capsys: LogCaptureFixture): """Tests the fatal_error() function.""" # -- Create the scons env. @@ -387,4 +392,151 @@ def test_log_functions(capsys): assert "Error: My fatal error\n" == captured.out -# Add test for color forcing on/off +def test_force_colors(capsys: LogCaptureFixture): + """Tests that the "force_colors" controls text coloring.""" + # -- Creating an env without force_colors defaul to false. + env = _make_test_env() + assert not force_colors(env) + + # -- Output a message and verify no ansi colors. + capsys.readouterr() # clear + msg(env, "xyz", fg="red") + captured = capsys.readouterr() + assert captured.out == "xyz\n" + + # -- Output a message and verify no ansi colors. + env = _make_test_env(extra_args={"force_colors": "False"}) + assert not force_colors(env) + + # -- Output a message and verify text is not colored. + capsys.readouterr() # clear + msg(env, "xyz", fg="red") + captured = capsys.readouterr() + assert captured.out == "xyz\n" + + # -- Creating an env without with force_colors = True + env = _make_test_env(extra_args={"force_colors": "True"}) + assert force_colors(env) + + # -- Output a message and verify text is colored. + capsys.readouterr() # clear + msg(env, "xyz", fg="red") + captured = capsys.readouterr() + assert captured.out == "\x1b[31mxyz\x1b[0m\n" + + +def test_get_constraint_file( + capsys: LogCaptureFixture, apio_runner: ApioRunner +): + """Test the get_constraint_file() function.""" + + with apio_runner.in_disposable_temp_dir(): + + env = _make_test_env() + + # -- If not .pcf files, should assume main name + extension and + # -- inform the user about it. + capsys.readouterr() # Reset capture + result = get_constraint_file(env, ".pcf", "my_main") + captured = capsys.readouterr() + assert "assuming 'my_main.pcf'" in captured.out + assert result == "my_main.pcf" + + # -- If a single .pcf file, return it. + Path("pinout.pcf").touch() + result = get_constraint_file(env, ".pcf", "my_main") + captured = capsys.readouterr() + assert captured.out == "" + assert result == "pinout.pcf" + + # -- If thre is more than one, exit with an error message. + Path("other.pcf").touch() + capsys.readouterr() # Reset capture + with pytest.raises(SystemExit) as e: + result = get_constraint_file(env, ".pcf", "my_main") + captured = capsys.readouterr() + assert e.value.code == 1 + assert "Error: Found multiple '*.pcf'" in captured.out + + +def test_get_programmer_cmd(capsys: LogCaptureFixture): + """Tests the function get_programmer_cmd().""" + + # -- Without a "prog" arg, expected to return "". This is the case + # -- when scons handles a command that doesn't use the programmer. + env = _make_test_env() + assert get_programmer_cmd(env) == "" + + # -- If prog is specified, expected to return it. + env = _make_test_env(extra_args={"prog": "my_prog aa $SOURCE bb"}) + assert get_programmer_cmd(env) == "my_prog aa $SOURCE bb" + + # -- If prog string doesn't contains $SOURCE, expected to exit with an + # -- error message. + env = _make_test_env(extra_args={"prog": "my_prog aa SOURCE bb"}) + with pytest.raises(SystemExit) as e: + capsys.readouterr() # Reset capturing. + get_programmer_cmd(env) + captured = capsys.readouterr() + assert e.value.code == 1 + assert "does not contain the '$SOURCE'" in captured.out + + +def test_make_verilator_config_builder(apio_runner: ApioRunner): + """Tests the make_verilator_config_builder() function.""" + + with apio_runner.in_disposable_temp_dir(): + + # -- Create a test scons env. + env = _make_test_env() + + # -- Call the tested function to create a builder. + builder = make_verilator_config_builder(env, "test-text") + + # -- Verify builder suffixes. + assert builder.suffix == ".vlt" + assert builder.src_suffix == [] + + # -- Create a target that doesn't exist yet. + assert not exists("hardware.vlt") + target = FS.File(FS(), "hardware.vlt") + + # -- Invoke the builder's action to create the target. + builder.action(target, [], env) + assert isfile("hardware.vlt") + + # -- Verify that the the file was created with the tiven text. + with open("hardware.vlt", "r", encoding="utf8") as f: + text = f.read() + + assert text == "test-text" + + +def test_get_source_files(apio_runner): + """Tests the get_source_files() function.""" + + with apio_runner.in_disposable_temp_dir(): + + # -- Create a test scons env. + env = _make_test_env() + + # -- Make files verilog src names (out of order) + Path("bbb.v").touch() + Path("aaa.v").touch() + + # -- Make files with testbench names (out of order) + Path("ccc_tb.v").touch() + Path("aaa_tb.v").touch() + + # -- Make files with non related names. + Path("ddd.vh").touch() + Path("eee.vlt").touch() + Path("subdir").mkdir() + Path("subdir/eee.v").touch() + + # -- Invoked the tested function. + srcs, testbenches = get_source_files(env) + + # -- Verify results. + assert srcs == ["aaa.v", "bbb.v"] + assert testbenches == ["aaa_tb.v", "ccc_tb.v"] From 7c94d5c3d28daa8413b67486ea7293ae3722d21b Mon Sep 17 00:00:00 2001 From: Zapta Date: Thu, 28 Nov 2024 20:56:04 -0800 Subject: [PATCH 9/9] Changed the scons code back to using the DefaultEnvironment. Now we just reset it in the tests before creating a new environment. This simplifies the code. --- apio/scons/scons_util.py | 37 ++++++++----------------- test/test_scons_util.py | 58 ++++------------------------------------ 2 files changed, 16 insertions(+), 79 deletions(-) diff --git a/apio/scons/scons_util.py b/apio/scons/scons_util.py index 11157979..55dc6d69 100644 --- a/apio/scons/scons_util.py +++ b/apio/scons/scons_util.py @@ -28,8 +28,8 @@ from SCons.Node import NodeList from SCons.Node.FS import File from SCons.Node.Alias import Alias -from SCons.Script import Environment from SCons.Script.SConscript import SConsEnvironment +from SCons.Script import DefaultEnvironment from SCons.Action import FunctionAction, Action from SCons.Builder import Builder import SCons.Defaults @@ -114,14 +114,20 @@ def is_windows(env: SConsEnvironment) -> bool: def create_construction_env(args: Dict[str, str]) -> SConsEnvironment: """Creates a scons env. Should be called very early in SConstruct.py""" - # Check that the default env is not used by mistake. SConstruct to use - # only our env. - check_default_scons_env_not_used() + # -- Make sure that the default environment doesn't exist, to make sure + # -- we create a fresh environment. This is important with pytest which + # -- can run multiple tests in the same python process. + # -- + # pylint: disable=protected-access + assert ( + SCons.Defaults._default_env is None + ), "DefaultEnvironment already exists" + # pylint: enable=protected-access # Create the env. We don't use the DefaultEnvironment (a singleton) to # allow the creation in pytest multiple test environments in the same # tesing process. - env: SConsEnvironment = Environment(ENV=os.environ, tools=[]) + env: SConsEnvironment = DefaultEnvironment(ENV=os.environ, tools=[]) # Add the args dictionary as a new ARGUMENTS var. assert env.get("ARGUMENTS") is None @@ -798,24 +804,3 @@ def set_up_cleanup(env: SConsEnvironment) -> None: # -- Associate all the files with the dummy target. env.Clean(dummy_target, files_to_clean) - - # -- Since the this function is called at the end of the SCOnstruct files, - # -- we use it to check that the SConstruct code didn't use by mistate - # -- the scons default env instead of the env we created in this file. - check_default_scons_env_not_used() - - -def check_default_scons_env_not_used(): - """A method to check that the default scons env has not been used. - We avoid using the default scons env because it's a singleton and thus, - don't play well with pytest where multiple indendepent scons envs - are created by tests in the same python process. This function asserts - that the default scons env was not instantiated by mistake, e.g. - by calling in SConstrut AllwaysBuild() instead of env.AlwaysBuild(). - """ - - # pylint: disable=protected-access - assert ( - SCons.Defaults._default_env is None - ), "Detected usage of scons default env." - # pylint: enable=protected-access diff --git a/test/test_scons_util.py b/test/test_scons_util.py index 3d4c3a2e..ccb8dca2 100644 --- a/test/test_scons_util.py +++ b/test/test_scons_util.py @@ -26,7 +26,6 @@ arg_bool, is_windows, force_colors, - check_default_scons_env_not_used, set_up_cleanup, map_params, fatal_error, @@ -63,9 +62,6 @@ def reset_scons_state() -> None: are running in the same process, we need to reset though variables before each test.""" - # -- We don't reset the _default_env, just make sure it was never used. - assert not SconsHacks.default_scons_env_exists() - # -- The Cons.Script.Main.OptionsParser variables contains the command # -- line options of scons. We reset them here and tests can access # -- them using SetOption() and GetOption(). @@ -83,33 +79,17 @@ def reset_scons_state() -> None: # -- Clear the SCons targets SCons.Environment.CleanTargets = {} - # -- Check again, just in case. - assert not SconsHacks.default_scons_env_exists() + # -- Reset the default scons env, if it exists. + # + # pylint: disable=protected-access + SCons.Defaults._default_env = None + # pylint: enable=protected-access @staticmethod def get_targets() -> Dict: """Get the scons {target -> dependencies} dictionary.""" return SCons.Environment.CleanTargets - @staticmethod - def default_scons_env_exists() -> bool: - """Tests if the default scons env was created. The default env is a - singleton that don't play well with pytest so we avoid it and use - instead an expclicit environment we create, and verify that the - default environement was not used by mistake, for example by - SConstruct calling AllwaysBuild() instead of env.AllwaysBuild(). - """ - # pylint: disable=protected-access - return SCons.Defaults._default_env is not None - # pylint: enable=protected-access - - @staticmethod - def clear_scons_default_env() -> None: - """Clear the scons default environment.""" - # pylint: disable=protected-access - SCons.Defaults._default_env = None - # pylint: enable=protected-access - def _make_test_env( args: Dict[str, str] = None, extra_args: Dict[str, str] = None @@ -134,9 +114,6 @@ def _make_test_env( # -- Use the apio's scons env creation function. env = create_construction_env(args) - # -- Check that the default scons env was not used. - check_default_scons_env_not_used() - return env @@ -264,31 +241,6 @@ def test_env_forcing_color(): assert not force_colors(env) -def test_default_env_check(): - """Teest that check_default_scons_env_not_used() throws an assertion - exception when the default scons env is used. - """ - # -- The starting point is having no default env. - assert not SconsHacks.default_scons_env_exists() - - # -- Should not throw an exception since the default env does not exist. - check_default_scons_env_not_used() - - # -- Create the scons default env. - SCons.Defaults.DefaultEnvironment(ENV={}, tools=[]) - assert SconsHacks.default_scons_env_exists() - - # -- Verify that the assertion in the function fails. - with pytest.raises(AssertionError) as e: - check_default_scons_env_not_used() - - # -- Verify the assertion text. - assert "Detected usage of scons default env" in str(e.value) - - # -- Restore the no-default-env state for the reset of the tests. - SconsHacks.clear_scons_default_env() - - def test_set_up_cleanup_ok(apio_runner: ApioRunner): """Tests the success path of set_up_cleanup()."""