Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Core: Fix pytest rebuilder and improve editable installation detection #108

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

mawildoer
Copy link
Contributor

@mawildoer mawildoer commented Oct 28, 2024

  • Fix bug where pytest builds would fail because of system vs. venv python difference
  • Improve editable installation detection robustness

Fixes # (issue)

Checklist

Please read and execute the following:

  • My code follows the coding guidelines of this project
  • My PR title is following the contribution guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I ran Black to format my code

Code of Conduct

By submitting this issue, you agree to follow our Code of Conduct:

@mawildoer mawildoer enabled auto-merge (squash) October 28, 2024 22:51
from typing import Callable

logger = logging.getLogger(__name__)


# Check if installed as editable
def is_editable_install():
site_packages = site.getsitepackages()
return not any((pathlib.Path(sp) / "faebryk").exists() for sp in site_packages)
distro = Distribution.from_name(__name__.split(".")[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hit an issue and apparently this is a more robust way to detect editable installation: https://stackoverflow.com/questions/43348746/how-to-detect-if-module-is-installed-in-editable-mode

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@@ -43,7 +48,7 @@ def _do(*args, **kwargs):
"cmake not found, needed for compiling c++ code in editable mode"
)

pybind11_dir = _do(["python", "-m", "pybind11", "--cmakedir"]).strip()
pybind11_dir = _do([sys.executable, "-m", "pybind11", "--cmakedir"]).strip()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a venv issues I had causing pytest failures

@mawildoer mawildoer force-pushed the mawildoer/osx-build-arch branch 4 times, most recently from 1777d88 to c9a921c Compare October 29, 2024 04:38
@mawildoer mawildoer force-pushed the mawildoer/osx-build-arch branch from c9a921c to 4d88d57 Compare October 29, 2024 04:41
@mawildoer mawildoer merged commit b270dbb into main Oct 29, 2024
6 checks passed
@mawildoer mawildoer deleted the mawildoer/osx-build-arch branch October 29, 2024 10:29

def test_add():
from faebryk.core.cpp import add
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing here yields vastly better errors on import failures. It's common/recommended with pytest anyway

@@ -1082,3 +1084,54 @@ def setdefault(self, key: T, default: U) -> U:
except KeyError:
self[key] = default
return default


def run_live(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a little help from our dear friend Claude to go a bit bananas here. Was very helpful for debugging. I'd dump the streams just afterwards, but it's quite hard to tell in what order something was sent to stdout or stderr like that

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants