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.

Sign up for GitHub

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

Jump to bottom

Potential fix for broken editable install #582

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Potential fix for broken editable install #582

wants to merge 1 commit into from

Conversation

maichmueller
Copy link

this small change in setuptools_wraps I believe fixes the broken editable install links due to the src directory of the package being set incorrectly (#579 and #546 ).
I am unsure if this change holds other consequences for the scikit-build process, so this PR could also just be seen as a hint towards what direction of change would fix the issue if one of the maintainers wants to take this on themselves.

@henryiii
Copy link
Contributor

This looks like it would be pretty easy to test. We could add a test for #546 pretty easily, see if it breaks, then try with this fix.

@henryiii henryiii added this to the 0.13.0 milestone Aug 23, 2021
@henryiii henryiii mentioned this pull request Aug 23, 2021
Open
@jwittbrodt
Copy link

I can confirm that the test project here reproduces the issue for me with the latest skbuild release (this scenario of a pybind11 package with a pure python sub-package is in fact exactly the use case where I first encountered the issue).

@maichmueller
Copy link
Author

thanks for confirming @jwittbrodt !

@henryiii
Copy link
Contributor

henryiii commented Aug 26, 2021 •
edited

But why is "test_classic_subproject" also failing? To me it looks like there still are some name mis-matches, from what I understand, only the editable install should fail?

Adding the fix commented out here still fails on both. I think there's a path problem in the setup here that needs to be solved before this is helpful.

@maichmueller
Copy link
Author

maichmueller commented Aug 26, 2021 •
edited

I can't tell why it is failing in the workflow. It is working in my docker environment on ubuntu with the current scikit-build (pip install . -v and then import pkg and import pkg.subpkg in a python shell).
The workflow output is not informative, however, and I am unsure about the specifics of the virtualenv setup for it. I merely copied your test case in your PR and added the relevant files to hopefully make it fail exactly as on my system. Can the pytest be run on the workflow with verbose output maybe?

@henryiii
Copy link
Contributor

Okay, I'lll try to help fix this soon. I can try running that locally, then figuring out what is different.

@henryiii
Copy link
Contributor

Looks like just the editable install is failing (at least on the one that has finished so far) :)

I've fixed the actions so it should run when you make an edit without me having to click approve.

@henryiii
Copy link
Contributor

henryiii commented Aug 30, 2021 •
edited

Curious, it seems to be that Windows passes the non-editable install check, but Linux does not. Are you on Windows too?

@maichmueller
Copy link
Author

Thanks for lifting the restrictions! That should make testing faster.

I am on a Windows machine but I am running the project in a ubuntu docker container (the test case with the dockerfile I am using is here: https://github.com/maichmueller/scikit_build_issue/blob/master/Dockerfile).

Which workflow run that succeded do you mean? It seems to me that all have failed so far?

PS

@henryiii
Copy link
Contributor

All workflows fail, because the editable install fails. I'm mostly looking at the non-editable install, which I'd expect to work. It seems to work on Windows, but not Linux. This might be due to Python 2, which only runs on non-Windows.

Still getting No module named 'pkg' (on both now that I've renabled the fix in this PR).

@maichmueller
Copy link
Author

Have you tested the case locally to see if it installs the pkg correctly there (for both types of install)?

@maichmueller
Copy link
Author

So I have tested my fix now locally on my Ubuntu 20.04 (py38) station and saw it fix the issue.
I have then tested this PR's fork of skbuild by running pytest on the relevant skbuild tests on this machine and saw the classic one not fail, but the editable one did. I can't really dig into what is happening during the tests, because I have no idea what is happening during the test setup. E.g. I can't..

  1. hook up the debugger onto pytest (getting a warning about sys.settrace having been modified)
  2. any changes to the skbuild source code are not executed in pytest (e.g. tried immediately returning out of setuptools_wraps.setup and that never is executed)
  3. Because of 2. can't print any variable values or other info.

How can I modify the code in a way that takes effect for the testcases? Without that, I can't quite dig any further into differences between my local setup and the test setup.

@henryiii
Copy link
Contributor

henryiii commented Sep 9, 2021

I'll try to help soon-ish, but https://scikit-hep.org/developer/pytest#running-pytest describes how to debug with pytest. You can use --trace to start a debugger at the beginning, or --pdb to start the debugger when it fails. You can use --lf to start at the last failed test (good for --trace). You can pass arguments to pytest through nox with --.

I think there was a caching issue that I've just fixed with the test.

@henryiii henryiii mentioned this pull request Dec 28, 2021
Open
@henryiii
Copy link
Contributor

The regular install works with/without, and the editable install does not work with or without the patch. :(

@henryiii henryiii modified the milestones: 0.13.0, 0.14.0 Feb 2, 2022
@maichmueller @henryiii
tests: add subdirectory package test
f3bfb97 
possible broken editable install fix

no copy of package_dir

revert back to old package_dir setting and add henrii test

copy paste error fix

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

update test project for breaking test case

now a cpp extension

add flake8 ignore for init.py

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

fix: py 2 (ugh)

Update test_issue546_editable_subproject.py

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

pkg import statement fix

try import of subpkg too

pybind11 module rename to _pkg as well

implement henry suggestions for init

test verbose install

cleaner pybind11 path search

fix: uncomment workaround for issue

fix: forgot to remove the original method

fix: tests failed due to caching

tests: minor cleanup

tests: try no build isolation for editable install

fix: do not remove added package dir

[pre-commit.ci] pre-commit autoupdate (scikit-build#684)

* [pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/psf/black: 22.1.0 → 22.3.0](psf/black@22.1.0...22.3.0)

* tests: support latest setuptools change

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Update __init__.py

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
This was referenced Apr 7, 2022
Open
Merged
@henryiii henryiii closed this Apr 7, 2022
@henryiii henryiii closed this Apr 7, 2022
@henryiii henryiii reopened this Apr 7, 2022
@henryiii henryiii reopened this Apr 7, 2022
@henryiii henryiii mentioned this pull request Apr 12, 2022
Open
@casperdcl casperdcl mentioned this pull request May 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henryiii henryiii henryiii left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@maichmueller @henryiii @jwittbrodt