From 644ae66b8d043f71d502a36747cbd48a05a66be7 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Wed, 15 May 2024 09:31:16 -0400 Subject: [PATCH] fix[tool]: fix cross-compilation issues, add windows CI (#4014) this commit fixes some `InputBundle` bugs on windows due to how path-manipulation operations are implemented. it also fixes compilation of cross-OS archives by settling on using posix paths as standard (this follows the ZIP archive spec, and is explicit in the python stdlib implementation of zipfile). this commit also enables a windows job in the CI for tests, to prevent bugs like these in the future and increase cross-OS coverage. references: - https://stackoverflow.com/a/8177003 - https://hg.python.org/cpython/file/2.7/Lib/zipfile.py#l295 - https://hg.python.org/cpython/file/2.7/Lib/zipfile.py#l1046 --------- Co-authored-by: Daniel Schiavini --- .github/workflows/test.yml | 35 +++++++++++++++++-- .../functional/builtins/codegen/test_slice.py | 2 +- tests/unit/compiler/test_input_bundle.py | 28 +++++++-------- .../venom/test_liveness_simple_loop.py | 4 ++- vyper/cli/vyper_json.py | 4 +-- vyper/compiler/__init__.py | 2 -- vyper/compiler/input_bundle.py | 14 +++++--- vyper/compiler/output_bundle.py | 20 ++++++++--- vyper/compiler/phases.py | 4 +-- vyper/semantics/analysis/module.py | 4 +-- 10 files changed, 82 insertions(+), 35 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3162b7ae6f..4b8cd937b9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -194,16 +194,47 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} file: ./coverage.xml + windows: + runs-on: windows-latest + strategy: + matrix: + python-version: [["3.12", "312"]] + evm-version: [shanghai] + evm-backend: [py-evm] + + name: "py${{ matrix.python-version[1] }}-windows-${{ matrix.evm-version }}-${{ matrix.evm-backend }}" + + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # we need the full history for setuptools_scm to infer the version + + - name: Set up Python ${{ matrix.python-version[0] }} + uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version[0] }} + cache: "pip" + + - name: Install dependencies + run: pip install .[test] + + - name: Run tests + run: > + pytest + -m "not fuzzing" + --evm-version ${{ matrix.evm-version }} + --evm-backend ${{ matrix.evm-backend }} + tests/ core-tests-success: if: always() # summary result from test matrix. # see https://github.community/t/status-check-for-a-matrix-jobs/127354/7 runs-on: ubuntu-latest - needs: tests + needs: [tests, windows] steps: - name: Check tests tests all succeeded - if: ${{ needs.tests.result != 'success' }} + if: ${{ needs.tests.result != 'success' || needs.windows.result != 'success' }} run: exit 1 diff --git a/tests/functional/builtins/codegen/test_slice.py b/tests/functional/builtins/codegen/test_slice.py index 5c2cc1b6ec..5ba1cd54f3 100644 --- a/tests/functional/builtins/codegen/test_slice.py +++ b/tests/functional/builtins/codegen/test_slice.py @@ -37,7 +37,7 @@ def slice_tower_test(inp1: Bytes[50]) -> Bytes[50]: def _fail_contract(code, opt_level, exceptions): settings = Settings(optimize=opt_level) with pytest.raises(exceptions): - compile_code(code, settings) + compile_code(code, settings=settings) @pytest.mark.parametrize("use_literal_start", (True, False)) diff --git a/tests/unit/compiler/test_input_bundle.py b/tests/unit/compiler/test_input_bundle.py index 74fd04f16e..b3b3a58bf8 100644 --- a/tests/unit/compiler/test_input_bundle.py +++ b/tests/unit/compiler/test_input_bundle.py @@ -56,13 +56,13 @@ def test_search_path_precedence(make_file, tmp_path, tmp_path_factory, input_bun file = ib.load_file("foo.vy") assert isinstance(file, FileInput) - assert file == FileInput(0, "foo.vy", filepaths[2], "contents 2") + assert file == FileInput(0, Path("foo.vy"), filepaths[2], "contents 2") with ib.search_path(tmpdir): file = ib.load_file("foo.vy") assert isinstance(file, FileInput) - assert file == FileInput(1, "foo.vy", filepaths[1], "contents 1") + assert file == FileInput(1, PurePath("foo.vy"), filepaths[1], "contents 1") # special rules for handling json files @@ -73,13 +73,13 @@ def test_load_abi(make_file, input_bundle, tmp_path): file = input_bundle.load_file("foo.json") assert isinstance(file, ABIInput) - assert file == ABIInput(0, "foo.json", path, contents, "some string") + assert file == ABIInput(0, PurePath("foo.json"), path, contents, "some string") # suffix doesn't matter path = make_file("foo.txt", contents) file = input_bundle.load_file("foo.txt") assert isinstance(file, ABIInput) - assert file == ABIInput(1, "foo.txt", path, contents, "some string") + assert file == ABIInput(1, PurePath("foo.txt"), path, contents, "some string") # check that unique paths give unique source ids @@ -89,23 +89,23 @@ def test_source_id_file_input(make_file, input_bundle, tmp_path): file = input_bundle.load_file("foo.vy") assert file.source_id == 0 - assert file == FileInput(0, "foo.vy", foopath, "contents") + assert file == FileInput(0, PurePath("foo.vy"), foopath, "contents") file2 = input_bundle.load_file("bar.vy") # source id increments assert file2.source_id == 1 - assert file2 == FileInput(1, "bar.vy", barpath, "contents 2") + assert file2 == FileInput(1, PurePath("bar.vy"), barpath, "contents 2") file3 = input_bundle.load_file("foo.vy") assert file3.source_id == 0 - assert file3 == FileInput(0, "foo.vy", foopath, "contents") + assert file3 == FileInput(0, PurePath("foo.vy"), foopath, "contents") # test source id is stable across different search paths with working_directory(tmp_path): with input_bundle.search_path(Path(".")): file4 = input_bundle.load_file("foo.vy") assert file4.source_id == 0 - assert file4 == FileInput(0, "foo.vy", foopath, "contents") + assert file4 == FileInput(0, PurePath("foo.vy"), foopath, "contents") # test source id is stable even when requested filename is different with working_directory(tmp_path.parent): @@ -126,22 +126,22 @@ def test_source_id_json_input(make_file, input_bundle, tmp_path): file = input_bundle.load_file("foo.json") assert isinstance(file, ABIInput) - assert file == ABIInput(0, "foo.json", foopath, contents, "some string") + assert file == ABIInput(0, PurePath("foo.json"), foopath, contents, "some string") file2 = input_bundle.load_file("bar.json") assert isinstance(file2, ABIInput) - assert file2 == ABIInput(1, "bar.json", barpath, contents2, ["some list"]) + assert file2 == ABIInput(1, PurePath("bar.json"), barpath, contents2, ["some list"]) file3 = input_bundle.load_file("foo.json") assert file3.source_id == 0 - assert file3 == ABIInput(0, "foo.json", foopath, contents, "some string") + assert file3 == ABIInput(0, PurePath("foo.json"), foopath, contents, "some string") # test source id is stable across different search paths with working_directory(tmp_path): with input_bundle.search_path(Path(".")): file4 = input_bundle.load_file("foo.json") assert file4.source_id == 0 - assert file4 == ABIInput(0, "foo.json", foopath, contents, "some string") + assert file4 == ABIInput(0, PurePath("foo.json"), foopath, contents, "some string") # test source id is stable even when requested filename is different with working_directory(tmp_path.parent): @@ -159,14 +159,14 @@ def test_mutating_file_source_id(make_file, input_bundle, tmp_path): file = input_bundle.load_file("foo.vy") assert file.source_id == 0 - assert file == FileInput(0, "foo.vy", foopath, "contents") + assert file == FileInput(0, PurePath("foo.vy"), foopath, "contents") foopath = make_file("foo.vy", "new contents") file = input_bundle.load_file("foo.vy") # source id hasn't changed, even though contents have assert file.source_id == 0 - assert file == FileInput(0, "foo.vy", foopath, "new contents") + assert file == FileInput(0, PurePath("foo.vy"), foopath, "new contents") # test the os.normpath behavior of symlink diff --git a/tests/unit/compiler/venom/test_liveness_simple_loop.py b/tests/unit/compiler/venom/test_liveness_simple_loop.py index e725518179..67752cdf0d 100644 --- a/tests/unit/compiler/venom/test_liveness_simple_loop.py +++ b/tests/unit/compiler/venom/test_liveness_simple_loop.py @@ -13,4 +13,6 @@ def foo(a: uint256): def test_liveness_simple_loop(): - vyper.compile_code(source, ["opcodes"], settings=Settings(experimental_codegen=True)) + vyper.compile_code( + source, output_formats=["opcodes"], settings=Settings(experimental_codegen=True) + ) diff --git a/vyper/cli/vyper_json.py b/vyper/cli/vyper_json.py index 4f73732687..beab06e3df 100755 --- a/vyper/cli/vyper_json.py +++ b/vyper/cli/vyper_json.py @@ -235,7 +235,7 @@ def get_output_formats(input_dict: dict) -> dict[PurePath, list[str]]: output_paths = [PurePath(path) for path in input_dict["sources"].keys()] else: output_paths = [PurePath(path)] - if str(output_paths[0]) not in input_dict["sources"]: + if output_paths[0].as_posix() not in input_dict["sources"]: raise JSONError(f"outputSelection references unknown contract '{output_paths[0]}'") for output_path in output_paths: @@ -317,7 +317,7 @@ def compile_from_input_dict( def format_to_output_dict(compiler_data: dict) -> dict: output_dict: dict = {"compiler": f"vyper-{vyper.__version__}", "contracts": {}, "sources": {}} for path, data in compiler_data.items(): - path = str(path) # Path breaks json serializability + path = path.as_posix() # Path breaks json serializability output_dict["sources"][path] = {"id": data["source_id"]} for k in ("ast_dict", "annotated_ast_dict"): diff --git a/vyper/compiler/__init__.py b/vyper/compiler/__init__.py index 9ed38885b4..e4c5bc49eb 100644 --- a/vyper/compiler/__init__.py +++ b/vyper/compiler/__init__.py @@ -105,8 +105,6 @@ def compile_from_file_input( # make IR output the same between runs codegen.reset_names() - # TODO: maybe at this point we might as well just pass a `FileInput` - # directly to `CompilerData`. compiler_data = CompilerData( file_input, input_bundle, diff --git a/vyper/compiler/input_bundle.py b/vyper/compiler/input_bundle.py index 51f1779119..a928989393 100644 --- a/vyper/compiler/input_bundle.py +++ b/vyper/compiler/input_bundle.py @@ -1,6 +1,6 @@ import contextlib import json -import os +import posixpath from dataclasses import asdict, dataclass, field from functools import cached_property from pathlib import Path, PurePath @@ -106,6 +106,8 @@ def _generate_source_id(self, resolved_path: PathLike) -> int: def load_file(self, path: PathLike | str) -> CompilerInput: # search path precedence tried = [] + if isinstance(path, str): + path = PurePath(path) for sp in reversed(self.search_paths): # note from pathlib docs: # > If the argument is an absolute path, the previous path is ignored. @@ -187,9 +189,13 @@ def _load_from_path(self, resolved_path: Path, original_path: Path) -> CompilerI return FileInput(source_id, original_path, resolved_path, code) -# wrap os.path.normpath, but return the same type as the input +# wrap os.path.normpath, but return the same type as the input - +# but use posixpath instead so that things work cross-platform. def _normpath(path): - return path.__class__(os.path.normpath(path)) + cls = path.__class__ + if not isinstance(path, str): + path = path.as_posix() + return cls(posixpath.normpath(path)) # fake filesystem for "standard JSON" (aka solc-style) inputs. takes search @@ -255,7 +261,7 @@ def _load_from_path(self, resolved_path: PurePath, original_path: PurePath) -> C # zipfile.BadZipFile: File is not a zip file try: - value = self.archive.read(str(resolved_path)).decode("utf-8") + value = self.archive.read(resolved_path.as_posix()).decode("utf-8") except KeyError: # zipfile literally raises KeyError if the file is not there raise _NotFound(resolved_path) diff --git a/vyper/compiler/output_bundle.py b/vyper/compiler/output_bundle.py index 13e74922a8..b93ecbd015 100644 --- a/vyper/compiler/output_bundle.py +++ b/vyper/compiler/output_bundle.py @@ -30,7 +30,11 @@ def _anonymize(p: str): segments.append(str(i)) else: segments.append(s) - return str(PurePath(*segments)) + + # the way to get reliable paths which reproduce cross-platform is to use + # posix style paths. + # cf. https://stackoverflow.com/a/122485 + return PurePath(*segments).as_posix() # data structure containing things that should be in an output bundle, @@ -58,7 +62,7 @@ def compiler_inputs(self) -> dict[str, CompilerInput]: sources = {} for c in inputs: - path = os.path.relpath(str(c.resolved_path)) + path = os.path.relpath(c.resolved_path) # note: there should be a 1:1 correspondence between # resolved_path and source_id, but for clarity use resolved_path # since it corresponds more directly to search path semantics. @@ -68,8 +72,8 @@ def compiler_inputs(self) -> dict[str, CompilerInput]: @cached_property def compilation_target_path(self): - p = self.compiler_data.file_input.resolved_path - p = os.path.relpath(str(p)) + p = PurePath(self.compiler_data.file_input.resolved_path) + p = os.path.relpath(p) return _anonymize(p) @cached_property @@ -255,6 +259,12 @@ def write_version(self, version: str): self.archive.writestr("MANIFEST/compiler_version", version) def output(self): - assert self.archive.testzip() is None self.archive.close() + + # there is a race on windows where testzip() will return false + # before closing. close it and then reopen to run testzip() + s = zipfile.ZipFile(self._buf) + assert s.testzip() is None + del s # garbage collector, cf. `__del__()` method. + return self._buf.getvalue() diff --git a/vyper/compiler/phases.py b/vyper/compiler/phases.py index 0de8e87c1a..10b4833e67 100644 --- a/vyper/compiler/phases.py +++ b/vyper/compiler/phases.py @@ -110,8 +110,8 @@ def _generate_ast(self): settings, ast = vy_ast.parse_to_ast_with_settings( self.source_code, self.source_id, - module_path=str(self.contract_path), - resolved_path=str(self.file_input.resolved_path), + module_path=self.contract_path.as_posix(), + resolved_path=self.file_input.resolved_path.as_posix(), ) if self.original_settings: diff --git a/vyper/semantics/analysis/module.py b/vyper/semantics/analysis/module.py index 06469ccef2..63eafdbaf4 100644 --- a/vyper/semantics/analysis/module.py +++ b/vyper/semantics/analysis/module.py @@ -888,8 +888,8 @@ def _parse_ast(file: FileInput) -> vy_ast.Module: ret = vy_ast.parse_to_ast( file.source_code, source_id=file.source_id, - module_path=str(module_path), - resolved_path=str(file.resolved_path), + module_path=module_path.as_posix(), + resolved_path=file.resolved_path.as_posix(), ) return ret