From dd1e756cd95593e4becbab2d9331daa0a68314a1 Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Mon, 9 Sep 2024 17:14:35 -0500 Subject: [PATCH 1/7] fix: bug --- src/ape/managers/project.py | 67 +++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/src/ape/managers/project.py b/src/ape/managers/project.py index ae62a12b96..e4b69ec223 100644 --- a/src/ape/managers/project.py +++ b/src/ape/managers/project.py @@ -2125,34 +2125,37 @@ def __getattr__(self, item: str) -> Any: message = getattr(err, "message", str(err)) did_append = False - all_files = get_all_files_in_directory(self.contracts_folder) - for path in all_files: - # Possibly, the user was trying to use a file name instead. - if path.stem != item: - continue + if item not in (self.manifest.contract_types or {}): + all_files = get_all_files_in_directory(self.contracts_folder) + for path in all_files: + # Possibly, the user was trying to use a file name instead. + if path.stem != item: + continue - if message and message[-1] not in (".", "?", "!"): - message = f"{message}." - message = ( - f"{message} However, there is a source file named '{path.name}', " - "did you mean to reference a contract name from this source file?" - ) - did_append = True - break - - # Possibly, the user does not have compiler plugins installed or working. - missing_exts = set() - for path in all_files: - if ext := get_full_extension(path): - if ext not in self.compiler_manager.registered_compilers: - missing_exts.add(ext) - - if missing_exts: - start = "Else, could" if did_append else "Could" - message = ( - f"{message} {start} it be from one of the " - "missing compilers for extensions: " + f'{", ".join(sorted(missing_exts))}?' - ) + if message and message[-1] not in (".", "?", "!"): + message = f"{message}." + + message = ( + f"{message} However, there is a source file named '{path.name}', " + "this file may no be compiling (see error above), or maybe you meant " + "to reference a contract name from this source file?" + ) + did_append = True + break + + # Possibly, the user does not have compiler plugins installed or working. + missing_exts = set() + for path in all_files: + if ext := get_full_extension(path): + if ext not in self.compiler_manager.registered_compilers: + missing_exts.add(ext) + + if missing_exts: + start = "Else, could" if did_append else "Could" + message = ( + f"{message} {start} it be from one of the " + "missing compilers for extensions: " + f'{", ".join(sorted(missing_exts))}?' + ) err.args = (message,) raise # The same exception (keep the stack the same height). @@ -2357,9 +2360,15 @@ def isolate_in_tempdir(self, **config_override) -> Iterator["LocalProject"]: else: # Add sources to manifest memory, in case they are missing. + original_sources = self._manifest.sources self._manifest.sources = dict(self.sources.items()) - with super().isolate_in_tempdir(**config_override) as project: - yield project + try: + with super().isolate_in_tempdir(**config_override) as project: + yield project + + finally: + # Restore original manifest sources in case compile failed. + self._manifest.sources = original_sources def unpack(self, destination: Path, config_override: Optional[dict] = None) -> "LocalProject": config_override = {**self._config_override, **(config_override or {})} From bcea3a804df10de0dfcefc4e472174c919f44efa Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Mon, 9 Sep 2024 17:43:12 -0500 Subject: [PATCH 2/7] refactor: better way --- src/ape/managers/project.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/ape/managers/project.py b/src/ape/managers/project.py index e4b69ec223..ef3ee8175e 100644 --- a/src/ape/managers/project.py +++ b/src/ape/managers/project.py @@ -2359,16 +2359,11 @@ def isolate_in_tempdir(self, **config_override) -> Iterator["LocalProject"]: yield self else: - # Add sources to manifest memory, in case they are missing. - original_sources = self._manifest.sources - self._manifest.sources = dict(self.sources.items()) - try: - with super().isolate_in_tempdir(**config_override) as project: - yield project - - finally: - # Restore original manifest sources in case compile failed. - self._manifest.sources = original_sources + sources = dict(self.sources.items()) + with super().isolate_in_tempdir(**config_override) as project: + # Add sources to manifest memory, in case they are missing. + project.manifest.sources = sources + yield project def unpack(self, destination: Path, config_override: Optional[dict] = None) -> "LocalProject": config_override = {**self._config_override, **(config_override or {})} From a294f6957083aa5b89d90fcd3e79fc27107887e0 Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Mon, 9 Sep 2024 17:49:11 -0500 Subject: [PATCH 3/7] test: add test --- tests/functional/test_project.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/functional/test_project.py b/tests/functional/test_project.py index 9528f147bb..5060a650f9 100644 --- a/tests/functional/test_project.py +++ b/tests/functional/test_project.py @@ -148,6 +148,22 @@ def test_isolate_in_tempdir(project): assert not tmp_project.manifest_path.is_file() +def test_isolate_in_tempdir_does_not_alter_sources(project): + # First, create a bad source. + new_src = project.contracts_folder / "newsource.json" + new_src.write_text("this is not json, oops") + + try: + with project.isolate_in_tempdir() as tmp_project: + # The new (bad) source should be in the temp project. + assert "src/newsource.json" in (tmp_project.manifest.sources or {}) + finally: + new_src.unlink() + + # Ensure "newsource" did not persist in the in-memory manifest. + assert "src/newsource.json" not in (project.manifest.sources or {}) + + def test_in_tempdir(project, tmp_project): assert not project.in_tempdir assert tmp_project.in_tempdir From 61133671e26b7e45cb18d33ad1759333d6dd70d5 Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Mon, 9 Sep 2024 18:02:50 -0500 Subject: [PATCH 4/7] test: fix tests --- src/ape/managers/project.py | 4 ++-- tests/functional/test_project.py | 30 +++++++++++++++++------------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/ape/managers/project.py b/src/ape/managers/project.py index ef3ee8175e..b97dfb0d30 100644 --- a/src/ape/managers/project.py +++ b/src/ape/managers/project.py @@ -2136,8 +2136,8 @@ def __getattr__(self, item: str) -> Any: message = f"{message}." message = ( - f"{message} However, there is a source file named '{path.name}', " - "this file may no be compiling (see error above), or maybe you meant " + f"{message} However, there is a source file named '{path.name}'. " + "This file may no be compiling (see error above), or maybe you meant " "to reference a contract name from this source file?" ) did_append = True diff --git a/tests/functional/test_project.py b/tests/functional/test_project.py index 5060a650f9..51eec8657a 100644 --- a/tests/functional/test_project.py +++ b/tests/functional/test_project.py @@ -150,18 +150,21 @@ def test_isolate_in_tempdir(project): def test_isolate_in_tempdir_does_not_alter_sources(project): # First, create a bad source. - new_src = project.contracts_folder / "newsource.json" - new_src.write_text("this is not json, oops") + with project.temp_config(contracts_folder="tests"): + new_src = project.contracts_folder / "newsource.json" + new_src.write_text("this is not json, oops") + project.sources.refresh() # Only need to be called when run with other tests. - try: - with project.isolate_in_tempdir() as tmp_project: - # The new (bad) source should be in the temp project. - assert "src/newsource.json" in (tmp_project.manifest.sources or {}) - finally: - new_src.unlink() + try: + with project.isolate_in_tempdir() as tmp_project: + # The new (bad) source should be in the temp project. + assert "tests/newsource.json" in (tmp_project.manifest.sources or {}), project.path + finally: + new_src.unlink() + project.sources.refresh() - # Ensure "newsource" did not persist in the in-memory manifest. - assert "src/newsource.json" not in (project.manifest.sources or {}) + # Ensure "newsource" did not persist in the in-memory manifest. + assert "tests/newsource.json" not in (project.manifest.sources or {}) def test_in_tempdir(project, tmp_project): @@ -216,8 +219,9 @@ def test_getattr_same_name_as_source_file(project_with_source_files_contract): expected = ( r"'LocalProject' object has no attribute 'ContractA'\. " r"Also checked extra\(s\) 'contracts'\. " - r"However, there is a source file named 'ContractA\.sol', " - r"did you mean to reference a contract name from this source file\? " + r"However, there is a source file named 'ContractA\.sol'\. " + r"This file may no be compiling (see error above), " + r"or maybe you meant to reference a contract name from this source file\? " r"Else, could it be from one of the missing compilers for extensions: .*" ) with pytest.raises(AttributeError, match=expected): @@ -603,7 +607,7 @@ def test_project_api_foundry_and_ape_config_found(foundry_toml): def test_get_contract(project_with_contracts): actual = project_with_contracts.get_contract("Other") - assert isinstance(actual, ContractContainer) + assert isinstance(actual, ContractContainer), f"{type(actual)}" assert actual.contract_type.name == "Other" # Ensure manifest is only loaded once by none-ing out the path. From 9b09e190ec799ce4cf16b7318a3cd60cce1f7240 Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Mon, 9 Sep 2024 18:04:57 -0500 Subject: [PATCH 5/7] test: fix test again --- src/ape/managers/project.py | 2 +- tests/functional/test_project.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ape/managers/project.py b/src/ape/managers/project.py index b97dfb0d30..dd9f1f5971 100644 --- a/src/ape/managers/project.py +++ b/src/ape/managers/project.py @@ -2137,7 +2137,7 @@ def __getattr__(self, item: str) -> Any: message = ( f"{message} However, there is a source file named '{path.name}'. " - "This file may no be compiling (see error above), or maybe you meant " + "This file may not be compiling (see error above), or maybe you meant " "to reference a contract name from this source file?" ) did_append = True diff --git a/tests/functional/test_project.py b/tests/functional/test_project.py index 51eec8657a..9da19395b2 100644 --- a/tests/functional/test_project.py +++ b/tests/functional/test_project.py @@ -220,7 +220,7 @@ def test_getattr_same_name_as_source_file(project_with_source_files_contract): r"'LocalProject' object has no attribute 'ContractA'\. " r"Also checked extra\(s\) 'contracts'\. " r"However, there is a source file named 'ContractA\.sol'\. " - r"This file may no be compiling (see error above), " + r"This file may not be compiling \(see error above\), " r"or maybe you meant to reference a contract name from this source file\? " r"Else, could it be from one of the missing compilers for extensions: .*" ) From 5758a7f0b83b647ecc541a1d7d2ac1b760ea6eb8 Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Tue, 10 Sep 2024 09:17:16 -0500 Subject: [PATCH 6/7] test: integ test --- src/ape_pm/compiler.py | 10 +--------- tests/integration/cli/test_compile.py | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/ape_pm/compiler.py b/src/ape_pm/compiler.py index 01f75b8185..297af2ad25 100644 --- a/src/ape_pm/compiler.py +++ b/src/ape_pm/compiler.py @@ -54,15 +54,7 @@ def compile( code = src_path.read_text() source_id = source_ids[path] - try: - # NOTE: Always set the source ID to the source of the JSON file - # to avoid manifest corruptions later on. - contract_type = self.compile_code(code, project=project, sourceId=source_id) - except CompilerError as err: - logger.warning( - f"Unable to parse {ContractType.__name__} from '{source_id}'. Reason: {err}" - ) - continue + contract_type = self.compile_code(code, project=project, sourceId=source_id) # NOTE: Try getting name/ ID from code-JSON first. # That's why this is not part of `**kwargs` in `compile_code()`. diff --git a/tests/integration/cli/test_compile.py b/tests/integration/cli/test_compile.py index 0020b074fa..a551f8fad2 100644 --- a/tests/integration/cli/test_compile.py +++ b/tests/integration/cli/test_compile.py @@ -147,6 +147,33 @@ def test_compile_when_sources_change(ape_cli, runner, integ_project, clean_cache assert "contracts/Interface.json" not in result.output +@skip_projects_except("multiple-interfaces") +def test_compile_when_sources_change_problematically(ape_cli, runner, integ_project, clean_cache): + """ + There was a bug when sources changes but had errors, that the old sources continued + to be used and the errors were swallowed. + """ + source_path = integ_project.contracts_folder / "Interface.json" + content = source_path.read_text() + assert "bar" in content, "Test setup failed - unexpected content" + + result = runner.invoke( + ape_cli, ("compile", "--project", f"{integ_project.path}"), catch_exceptions=False + ) + assert result.exit_code == 0, result.output + + # Change the contents of a file in a problematic way. + source_path = integ_project.contracts_folder / "Interface.json" + modified_source_text = source_path.read_text().replace("{", "BRACKET") + source_path.unlink() + source_path.touch() + source_path.write_text(modified_source_text, encoding="utf8") + result = runner.invoke( + ape_cli, ("compile", "--project", f"{integ_project.path}"), catch_exceptions=False + ) + assert result.exit_code != 0, result.output + + @skip_projects_except("multiple-interfaces") def test_compile_when_contract_type_collision(ape_cli, runner, integ_project, clean_cache): source_path = integ_project.contracts_folder / "Interface.json" From 970855be83c23c65e3788192c2c9be8cc651cac6 Mon Sep 17 00:00:00 2001 From: Juliya Smith Date: Tue, 10 Sep 2024 09:43:19 -0500 Subject: [PATCH 7/7] chore: retry docs