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

Fix #21: Add setup files to create Python package #28

Merged
merged 4 commits into from
Dec 21, 2018
Merged

Conversation

nvs-abhilash
Copy link
Owner

Added setup.py file, and MANIFEST.in file to create a .whl file.
This helps in making the CorrectMe as a package.
Run python setup.py sdist bdist_wheel to get the wheel in dist directory.
This fixed #21.

@nvs-abhilash
Copy link
Owner Author

nvs-abhilash commented Dec 5, 2018

@piyush-jaiswal The details of testing the PR is given in the README.md in the Installation section.

@nvs-abhilash nvs-abhilash force-pushed the installation branch 3 times, most recently from 0fd56e7 to 7084b0e Compare December 5, 2018 16:28
README.md Outdated
conda create -n cm_env python=3.6 pip
source activate cm_env # activate cm_env for windows
cd CorrectMe # The cloned repository directory
python -m pip install --user --upgrade setuptools wheel
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't figure out why we need to build wheels manually. Couldn't we simply use these commands instead - pip install <path_to_cloned_repo> or python setup.py install?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, you are right! My fault, did not do much digging on setup.py. Updated the README, though I was not able to install using git directly. I guess I am having some syntax issues. Do you know any way to do it directly from git?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should work pip install git+https://github.com/nvs-abhilash/CorrectMe.git.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It doesn't seem to be working for me. Getting this error:
error_correctme

This seems to be a issue with PIP: pypa/pip#5271

Have you been facing similar issue or is it working for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The git URL is for the master branch, which doesn't have setup.py yet, hence the error. You have to specify the branch in the URL if its not master. Try this pip install git+https://github.com/nvs-abhilash/CorrectMe.git@installation.

@piyush-jaiswal
Copy link
Collaborator

piyush-jaiswal commented Dec 8, 2018

Since we only support python3, we could also use the python_requires argument of setup from setuptools, concretely python_requires='>=3'

@piyush-jaiswal
Copy link
Collaborator

Should we include main.py in the package as well?

Add setup.py file and MANIFEST.in file for making installation possible.
Modified README.md to incorporate the new style of install.
Run `pip install .` to get the library installed.

This fixed #21.
@nvs-abhilash
Copy link
Owner Author

Not sure how to add main.py to the package, as an example script or something? Should we rename it to something more meaningful inside package? We also would need a way to add the data folder to the package as well for main.py. Do you have some suggestions?

setup.py Show resolved Hide resolved
README.md Outdated

Installation using sources seems to work fine with version `kivy==1.10.1`
To test your installation, move the `main.py` file from the repository to another location, and run it.
Copy link
Owner Author

Choose a reason for hiding this comment

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

How can we modify this, now that main.py is part of the module itself.

@piyush-jaiswal
Copy link
Collaborator

piyush-jaiswal commented Dec 9, 2018

Hey @nvs-abhilash , I included data and main.py in setup as well. Could you take a look?

Currently main.py is installed inside site-packages outside of the correctme package. Should we change this behaviour and include main.py inside the correctme package itself?

The data folder is installed outside of dist-packages, on the same level as lib, but is importable by main.py. Can you think of a better solution?

@piyush-jaiswal
Copy link
Collaborator

Renamed main.py to Correctme.py and included in the installation. Included data folder to be part of the package. @nvs-abhilash, let me know if we need more changes.

@piyush-jaiswal
Copy link
Collaborator

Hey @nvs-abhilash, LGTM. Should I merge this?

@nvs-abhilash
Copy link
Owner Author

Sorry did not get a chance to test the installation properly. Will do it by tomorrow, and then let's merge it.

@piyush-jaiswal
Copy link
Collaborator

Yeah sure. Take your time. I just thought you were waiting on me to merge this :)

@nvs-abhilash
Copy link
Owner Author

Hi @piyush-jaiswal, finally got chance to look at it. Though the installation was working fine, but I was still confused on how to call CorrectMe.py and when I examined the installation I found out that it was getting installed outside the package, not sure whether it was necessary.

Then after some research, I stumbled across this article: python-apps-the-right-way-entry_points-and-scripts.

The above article showed how we can use python scripts as executable from the command prompt. And I guess this would allow us to call something like correctme-gui from the terminal itself and it would work as before!! I think this is awesome and exactly what we needed 🥇 . I have updated the setup.py file and moved CorrectMe.py to bin folder inside the package.

I request you to check if it is working for you or not, and suggest further improvements to the way I wrote it (maybe some naming choices), then let's close this.

@piyush-jaiswal
Copy link
Collaborator

Yup, LGTM @nvs-abhilash 🍾. Everything seems to be working and this certainly seems the way to go. Could you kindly update the README accordingly reflecting these changes?

@nvs-abhilash
Copy link
Owner Author

Done editing the docs. I think we should merge this branch now, as we have changed a lot of file structure from the master branch.

@nvs-abhilash nvs-abhilash merged commit fa3a1e1 into master Dec 21, 2018
@nvs-abhilash nvs-abhilash deleted the installation branch December 21, 2018 15:25
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.

No form of installation available
2 participants