Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: contract type collision issues from deleted files and empty file issues #1818

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/ape/managers/compilers.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,18 @@ def compile(
cached_manifest = self.project_manager.local_project.cached_manifest

# Load past compiled contracts for verifying type-collision and other things.
already_compiled_contracts = (
(cached_manifest.contract_types or {}) if cached_manifest else {}
)
already_compiled_paths = [
contracts_folder / x.source_id
for x in already_compiled_contracts.values()
if x.source_id
]
already_compiled_contracts: Dict[str, ContractType] = {}
already_compiled_paths: List[Path] = []
for name, ct in ((cached_manifest.contract_types or {}) if cached_manifest else {}).items():
if not ct.source_id:
continue

_file = contracts_folder / ct.source_id
if not _file.is_file():
continue

already_compiled_contracts[name] = ct
already_compiled_paths.append(_file)

exclusions = self.config_manager.get_config("compile").exclude
for extension in extensions:
Expand Down
14 changes: 11 additions & 3 deletions src/ape/managers/project/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ def _check_needs_compiling(self, source_path: Path) -> bool:
# ethpm_types strips trailing white space and ensures
# a newline at the end so content so `splitlines()` works.
# We need to do the same here for to prevent the endless recompiling bug.
content = f"{source_file.read_text('utf8').rstrip()}\n"
text = source_file.read_text("utf8").rstrip()
content = f"{text}\n" if text else ""

checksum = compute_checksum(content.encode("utf8"), algorithm=cached_checksum.algorithm)
return checksum != cached_checksum.hash # Contents changed

Expand Down Expand Up @@ -177,7 +179,11 @@ def create_manifest(
)
)

manifest = self.manifest if use_cache else PackageManifest()
if use_cache:
manifest = self.manifest
else:
self._contracts = None
manifest = PackageManifest()

# Generate sources and contract types.
project_sources = _ProjectSources(
Expand Down Expand Up @@ -211,7 +217,9 @@ def create_manifest(
# Is cached.
return self.manifest

def _compile(self, project_sources: _ProjectSources) -> Dict[str, ContractType]:
def _compile(
self, project_sources: _ProjectSources, use_cache: bool = True
) -> Dict[str, ContractType]:
def _compile_sources(proj_srcs: _ProjectSources) -> Dict[str, ContractType]:
contracts_folder = self.contracts_folder
srcs_to_compile = proj_srcs.sources_needing_compilation
Expand Down
8 changes: 4 additions & 4 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,9 @@ def zero_address():
@pytest.fixture
def ape_caplog(caplog):
class ApeCaplog:
def __init__(self):
def __init__(self, caplog_level: LogLevel = LogLevel.WARNING):
self.messages_at_start = list(caplog.messages)
self.set_levels()
self.set_levels(caplog_level=caplog_level)

def __getattr__(self, name: str) -> Any:
return getattr(caplog, name)
Expand Down Expand Up @@ -434,9 +434,9 @@ def head(self) -> str:
return caplog.messages[-1] if len(caplog.messages) else ""

@classmethod
def set_levels(cls):
def set_levels(cls, caplog_level: LogLevel = LogLevel.WARNING):
logger.set_level(LogLevel.INFO)
caplog.set_level(LogLevel.WARNING)
caplog.set_level(caplog_level)

def assert_last_log(self, message: str):
assert message in self.head, self.fail_message
Expand Down
16 changes: 13 additions & 3 deletions tests/functional/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -676,9 +676,19 @@ def mock_compile(paths, base_path=None):
if path.suffix == mock.ext:
name = path.stem
code = HexBytes(123).hex()
contract_type = ContractType(
contractName=name, abi=[], deploymentBytecode=code, sourceId=path.name
)
data = {
"contractName": name,
"abi": [],
"deploymentBytecode": code,
"sourceId": path.name,
}

# Check for mocked overrides
overrides = mock.overrides
if isinstance(overrides, dict):
data = {**data, **overrides}

contract_type = ContractType.model_validate(data)
result.append(contract_type)

return result
Expand Down
81 changes: 79 additions & 2 deletions tests/functional/test_project.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import os
import random
import shutil
import string
import tempfile
from pathlib import Path

import pytest
Expand All @@ -11,6 +14,7 @@

from ape import Contract
from ape.exceptions import ProjectError
from ape.logging import LogLevel
from ape.managers.project import BrownieProject

WITH_DEPS_PROJECT = (
Expand Down Expand Up @@ -119,7 +123,7 @@ def test_extract_manifest(project_with_dependency_config):


def test_cached_manifest_when_sources_missing(
ape_project, manifest_with_non_existent_sources, existing_source_path, caplog
ape_project, manifest_with_non_existent_sources, existing_source_path, ape_caplog
):
"""
Show that if a source is missing, it is OK. This happens when changing branches
Expand All @@ -143,7 +147,7 @@ def test_cached_manifest_when_sources_missing(

# Show the contract type does not get added and we don't get the corrupted manifest.
assert not any(ct.name == name for ct in manifest.contract_types.values())
assert not any("corrupted. Re-building" in msg for msg in caplog.messages)
assert not any("corrupted. Re-building" in msg for msg in ape_caplog.messages)


def test_create_manifest_when_file_changed_with_cached_references_that_no_longer_exist(
Expand Down Expand Up @@ -172,6 +176,44 @@ def test_create_manifest_when_file_changed_with_cached_references_that_no_longer
assert manifest


def test_create_manifest_empty_files(compilers, mock_compiler, config, ape_caplog):
"""
Tests again a bug where empty contracts would infinitely compile.
"""

# Using a random name to prevent async conflicts.
letters = string.ascii_letters
name = "".join(random.choice(letters) for _ in range(10))

with tempfile.TemporaryDirectory() as temp_dir:
base_dir = Path(temp_dir)
contracts = base_dir / "contracts"
contracts.mkdir()
file_1 = contracts / f"{name}.__mock__"
file_1.write_text("")

with config.using_project(base_dir) as proj:
compilers.registered_compilers[".__mock__"] = mock_compiler

# NOTE: Set levels as close to the operation as possible
# to lessen chance of caplog race conditions.
ape_caplog.set_levels(caplog_level=LogLevel.INFO)

# Run twice to show use_cache=False works.
proj.local_project.create_manifest()
manifest = proj.local_project.create_manifest(use_cache=False)

assert name in manifest.contract_types
assert f"{name}.__mock__" in manifest.sources

ape_caplog.assert_last_log(f"Compiling '{name}.__mock__'.")
ape_caplog.clear()

# Ensure is not double compiled!
proj.local_project.create_manifest()
assert f"Compiling '{name}.__mock__'." not in ape_caplog.head


def test_meta(temp_config, project):
meta_config = {
"meta": {
Expand Down Expand Up @@ -470,6 +512,41 @@ def test_load_contracts(project_with_contract):
assert contracts == project_with_contract.contracts


def test_load_contracts_after_deleting_same_named_contract(config, compilers, mock_compiler):
"""
Tests against a scenario where you:

1. Add and compile a contract
2. Delete that contract
3. Add a new contract with same name somewhere else

Test such that we are able to compile successfully and not get a misleading
collision error from deleted files.
"""

with tempfile.TemporaryDirectory() as temp_dir:
path = Path(temp_dir)
contracts = path / "contracts"
contracts.mkdir()
init_contract = contracts / "foo.__mock__"
init_contract.write_text("LALA")
with config.using_project(path) as proj:
compilers.registered_compilers[".__mock__"] = mock_compiler
result = proj.load_contracts()
assert "foo" in result

# Delete file
init_contract.unlink()

# Create new contract that yields same name as deleted one.
new_contract = contracts / "bar.__mock__"
new_contract.write_text("BAZ")
mock_compiler.overrides = {"contractName": "foo"}

result = proj.load_contracts()
assert "foo" in result


def test_add_compiler_data(project_with_dependency_config):
# NOTE: Using different project than default to lessen
# chance of race-conditions from multi-process test runners.
Expand Down
Loading