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

Refactoring of setup.py #1282

Merged
merged 3 commits into from
Nov 1, 2013
Merged

Refactoring of setup.py #1282

merged 3 commits into from
Nov 1, 2013

Conversation

rshk
Copy link
Contributor

@rshk rshk commented Oct 18, 2013

I refactored setup.py a bit:

  • Fixed some pep8 issues
  • Moved entry points to "dictionary" format, that's nicer to handle than an embedded ini file
  • Moved dependencies inside the install_requires, so you can just python setup.py install or pip install git+https://github.com/rshk/ckan.git.

@ghost ghost assigned nigelbabu Oct 22, 2013
@nigelbabu
Copy link
Contributor

I'm happy with all the changes except for moving the requirements inside setup.py. If you can revert that commit, I'll merge this in. For now, moving it inside setup.py makes it harder to update and we're having that discussion elsewhere.

Can also merge in master into this branch?

@rshk
Copy link
Contributor Author

rshk commented Nov 1, 2013

Ok, I'm cherry-picking 7ab8bd9 and 1ca945f on top of the current okfn:master and updating PR.

For now, moving it inside setup.py makes it harder to update and we're having that discussion elsewhere.

How so? I moved them inside setup.py to make it easier to update... this sounds strange to me.
Is there any problem I'm not aware of?

With "vanilla" version, I have to do:

pip install -r requirements.txt
python setup.py install

While, when having requirements in setup.py, installing/upgrading is just matter of a python setup.py install.

Actually, I use a local repository with tarballs of dependencies, so I use this:

pip install -f /path/to/deps/directory/ --no-index /path/to/ckan -r /path/to/ckan/requirements.txt

while, with requirements in setup.py, I could remove the -r .../requirements.txt part

@nigelbabu
Copy link
Contributor

No, no. I meant makes the requirements list itself harder to update and freeze. I don't really have a solid reason to give you, to be honest. It's just we're all little weary of having them in setup.py. We also have multiple sets of requirements.txt and putting them in setup.py makes it harder, for instance to just generate documentation without installing CKAN.

If we pin something in setup.py its like saying it has to be that version, when in all likelihood a later version would work, but we a have just not tested it yet. setup.py deps are very good for libraries but not really for application, especially ones with extensions.

Again, I understand these are not concrete reasons. The general feeling in the team is that we'd prefer the flexibility offered by using requirements.txt files.

If you can fix the pull requests with just the other two commits, I'll merge it in right away.

* Some pep8 fixes
* Entry points converted in dictionary format
@rshk
Copy link
Contributor Author

rshk commented Nov 1, 2013

Ok, here it is a new commit, based on top of the current master, with only the pep8/entry_points changes.

@rshk
Copy link
Contributor Author

rshk commented Nov 1, 2013

For the requirements thing: most applications put (loosely pinned) dependencies in setup.py and dependencies for development/tests in extras_require. For example:

setup(
    ...
    install_requires=[
        'Pylons==0.9.7',
        'SQAlchemy >= 0.7, <0.8',
        ...
    ],
    extras_require={
        'devel': ['docutils', 'pep8', ...],
        'docs': ['sphinx'],
    },
    ...
)

Pinning is good as you are sure the installed version is the right one, but it has drawbacks, such as conflicts in case, for example, a plugin uses another pinned version, so good practice is to pin just a major/minor version, when needed.

nigelbabu added a commit that referenced this pull request Nov 1, 2013
Refactoring of setup.py
@nigelbabu nigelbabu merged commit 2ccbea6 into ckan:master Nov 1, 2013
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