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

Modularized and added support for setuptools #28

Closed
wants to merge 1 commit into from

Conversation

yarda
Copy link

@yarda yarda commented Oct 13, 2016

This is my attempt to modularize the code and add support for setuptools. This ease downstream distro packaging. I tried to sanely set 'setup.py' metadata, but it may still need some improvements. I also changed all local imports to relative path, because recently there was opened downstream Fedora bug: https://bugzilla.redhat.com/show_bug.cgi?id=1383513 regarding conflicting name with the Event package. While I have been trying to fix this bug, I have decided to also fix the modularization.

@yarda
Copy link
Author

yarda commented Oct 13, 2016

Also added src/version.py to track the package version on one place.

@EarToEarOak
Copy link
Owner

Thanks,
I have no experience of setuptools and was wondering if it's possible to do this without moving rtlsdr_scan.py to the bin directory? This will break the Windows installer and will also confuse users who are used to running it from the src directory.

@yarda
Copy link
Author

yarda commented Oct 18, 2016

I will check the possibility. The standard way is to provide the standalones in so called scripts directory, but hopefully I will come with some compatible solution.

@yarda
Copy link
Author

yarda commented Oct 18, 2016

Yup, it seems to work, I will update the patch. One question: could the main executable file be named 'rtlsdr_scan', i.e. without the '.py' suffix? But no problem for me to rename it in the downstream install script.

@EarToEarOak
Copy link
Owner

Great.
For the moment I'd prefer to keep the '.py' extension to reduce confusion, but I see the advantage to removing it.

@yarda
Copy link
Author

yarda commented Oct 18, 2016

No problem, push forced the new patch which keeps the suffix, I will rename it downstream. Also feel free to update the setup, currently it doesn't handle the resources, but I think it's minor problem at the moment.

Signed-off-by: Jaroslav Škarvada <jskarvad@redhat.com>
@EarToEarOak
Copy link
Owner

The res directory is required in the distribution otherwise it will crash when it can't find the button bitmaps. The server portion also uses files from here as well.

@yarda
Copy link
Author

yarda commented Oct 20, 2016

Yes, I side-install it downstream. But I think it could be also done by setuptools. In such case you could simply run:
python setup.py build && python setup.py install

And all would be setup no matter of the platform / OS used.

Currently I do:
python setup.py build && python setup.py install && cp -r res TARGETDIR

But I think this is minor, we could extend the setup.py later. I am not expert of setuptools, but I think it should be possible.

@EarToEarOak
Copy link
Owner

I think it's preferable to have setuptools do all the installing, rather than manually copying the resources. It would also mean the egg file would include these files for distribution.

@EarToEarOak
Copy link
Owner

I've had a quick play with including the resource files. Looks like the directory structure needs to be made more package-like; renaming src to rtlsdr-scanner and moving res into this.

The functions which load the resources will then need to check if setuptools was used and then load them as described here.

@EarToEarOak
Copy link
Owner

I've added setuptools support in 9db6892 (and a few bug squashes after that). I've also uploaded the package to PyPi.

As I suspected they key was to turn it into a proper Python package, the rest came fairly easily after that.

As the events.py is a sub-module of rtlsdr-scanner it no longer seems to (or should) cause the Fedora import error.

Although I didn't merge your pull I found your modifications to be useful and have to thank you for all your help.

@yarda
Copy link
Author

yarda commented Jan 13, 2017

OK, thanks for info, I will update the package in Fedora, so closing the pull request.

@yarda yarda closed this Jan 13, 2017
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.

None yet

2 participants