Pebaz / nimporter

Compile Nim Extensions for Python On Import!
MIT License
829 stars 33 forks source link

package_dir setting in setup is not respected #46

Closed lhupfeldt closed 3 years ago

lhupfeldt commented 3 years ago

I have

package_dir={'mypackagename': 'src'},

in my setup.py

But nim compiled (.so) files are installed under .../site-packages/src instead of .../site-packages/mypackagename

Pebaz commented 3 years ago

I think this has to do with the suggested project structure mentioned in the readme. Although I'll have to test package_dir myself to be sure, I believe Nimporter imposes an output structure so it works without any fiddling. Let me know if the suggested project structure works for you. Namely, having a "src" dir is not recommended and may not work with all Nimporter features.

lhupfeldt commented 3 years ago

I find that having a "src" dir works better than having a "mypackage" dir. E.g. it makes sure that pytest actually run from the installed package, not accidentally finding files directly in the source "mypackage" dir. "src" also fits with the nim structure. This is an exixting python project, I hope I will not have to restructure it to introduce nim.

Pebaz commented 3 years ago

Ah yes I understand, I don't know why Pytest can't just look for a top-level package and assume that to be the source directory.

If you send me your project's GitHub URL I'd be willing to take a look and make a recommendation.

lhupfeldt commented 3 years ago

Pytest works with both structures. I just think that having a src dir different from the package name and using a test driver like nox, so that tests are run against the installed package instead of the sources, gives the least amount of weird surprises. This is the project I'm working on: file_groups. It uses setup.py, but should probably be converted to setup.cfg. Then you could also read any relevant configuration from there. My goal was actually to convert the entire package to nim (which uses src) and provide both a nimble and a pypi package. Given lack of type support in nimpy (see #45) this may not be feasible at this time.

Pebaz commented 3 years ago

After looking at your project (super cool by the way), I definitely recommend not going with a src based project structure. The reason why is because Nimporter was built around using a main Python package name rather than a directory that just happens to contain Python code.

Here are a couple solutions I have been using at work to get around Pytest not fixing PYTHONPATH to let tests import the application modules:

# For `src` based projects, place this code within `tests/__init__.py`
# It will get run prior to any unit tests by Pytest so the tests will
# be able to successfully import the application's modules
import sys
from pathlib import Path

sys.path.append(str(Path(__file__).parent.parent / 'src'))
# For `<main package>` based projects, place this code within
# the `tests/__init__.py`

import sys
from pathlib import Path

# The `/ 'src'` is removed to allow the package itself to be imported
sys.path.append(str(Path(__file__).parent.parent))

I know this goes against your preference and honestly I wish I would have built Nimporter to support more types of structures initially but it'd take a pretty decent rewrite to fix now :/

If you encounter any issues getting the Nim and Python projects to talk to each other please reopen this issue and I'd love to help in any way I can. :)

lhupfeldt commented 3 years ago

Thank you for taking time to elaborate.

I find that having pytest not find files in src is one of the benefits of that structure. I have stopped messing with sys.path in my test setup, I now always invoke pytest through nox so that it runs against the installed package. It just seems to give the fewest problems.

My goal was to turn the entire package into nim while at the same time exporting it as Python, but since nimpy does not handle types, it does not seen feasible at this time. So now I'm just having fun with nim. If I stick with it, I may take a look at your package later. I would assume it should be possible to add some configuration telling it the package name and the source directory, even if it cannot easily be gotten from setup.py.