Skip to content

Conversation

@jpgill86
Copy link
Contributor

This change is needed for setup.py to discover the neo/rawio/neuralynxrawio directory. Otherwise, it is excluded from non-development installs (python setup.py install and pip install .), wheels (python setup.py bdist_wheel), and source distributions (python setup.py sdist).

Note that this was not a problem for development-mode installations (python setup.py develop and pip install -e .) because when installing from the repo the NeuralynxRawIO directory is, of course, already there. It's only when the other methods are used that the directory fails to be copied to the correct final location (e.g., into site-packages or the source tarball).

The automated CircleCI and Travis CI tests use pip install ., so one would expect them to catch this problem. However, this slipped past them. I think that's because tests are executed from the repo's top-level directory, so neo/rawio/neuralynxrawio can be located in the working directory, even if it isn't in site-packages.

The only reason I noticed this is because of #930. ReadTheDocs does not change its working directory to the package's top-level before installing (instead, it installs with a lengthy path: /home/docs/checkouts/readthedocs.org/user_builds/neo/envs/latest/bin/python /home/docs/checkouts/readthedocs.org/user_builds/neo/checkouts/latest/setup.py install --force). Consequently, it cannot locate the NeuralynxRawIO directory when it tries to import neo after installation, and autodoc fails to compile the module and class documentation.

@jpgill86
Copy link
Contributor Author

Neo can avoid a problem like this in the future if ever new sub-packages are introduced by using setuptool's find_packages(), which will automatically detect all sub-packages. Presently, it returns the same list that is hard-coded into setup.py, including neo.rawio.neuralynxrawio, so there are none to filter out:

$ cd python-neo
$ python -c "from setuptools import find_packages; print(find_packages())"
['neo', 'neo.core', 'neo.io', 'neo.rawio', 'neo.test', 'neo.rawio.neuralynxrawio', 'neo.test.coretest', 'neo.test.iotest', 'neo.test.rawiotest']

I will push another commit that uses find_packages().

@JuliaSprenger
Copy link
Member

Hi @jpgill86,
yes, that's indeed a long-needed fix. Thanks for the implementation!

@JuliaSprenger JuliaSprenger merged commit edf6913 into NeuralEnsemble:master Jan 19, 2021
@jpgill86
Copy link
Contributor Author

Thanks @JuliaSprenger!

@jpgill86 jpgill86 deleted the setup-find-neuralynx branch January 19, 2021 14:21
@jpgill86
Copy link
Contributor Author

Oh, I see now what you were referring to @JuliaSprenger: #917. I guess that can be closed now?

@JuliaSprenger
Copy link
Member

JuliaSprenger commented Jan 19, 2021

Almost, I still think the import pattern is nicer in #917, as it preserves existing import patterns. @jpgill86 What do you think?

@jpgill86
Copy link
Contributor Author

I agree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants