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

Reintroduce flit-less setup.py #430

Merged
merged 5 commits into from
Jan 8, 2016
Merged

Reintroduce flit-less setup.py #430

merged 5 commits into from
Jan 8, 2016

Conversation

jdfreder
Copy link
Contributor

@jdfreder jdfreder commented Jan 6, 2016

@ellisonbg asked me to open this PR because the flit based setup.py prohibited him from doing some normal pip operations, see his comment in #409 . I mostly used the old setup.py that existed before flit was added, however some modifications were required to reflect the new state of nbgrader.

This PR does not remove flit.

closes #428

@jdfreder jdfreder added this to the 0.3.0 milestone Jan 6, 2016
@jdfreder
Copy link
Contributor Author

jdfreder commented Jan 6, 2016

cc @takluyver @Carreau , alternatively we could look at making flit work for the things @ellisonbg is having trouble with (I think are summarized in full in #409). I do prefer the syntax of flit way more than setup.py!

@takluyver
Copy link
Member

I am working on a way to install packages using flit from Github (see pypa/flit#65). But it doesn't sound like that will do everything that @ellisonbg wants. In particular, I don't plan to support installing a flit package from source without flit (though of course once you've made a wheel you can easily install it with pip).

I don't recommend having both flit and setuptools packaging in the same repo - there's no particular conflict, but it means you'll be duplicating information and risking it getting out of sync.

@jhamrick
Copy link
Member

jhamrick commented Jan 6, 2016

Thanks for starting this, and sorry I haven't had time to work on this myself (I am totally swamped preparing for my quals at the moment, which are in less than two weeks). It looks like you need to add back in the nbgrader script as well for this to work.

I do like the syntax of flit way better, too, but I would prefer to not have to maintain metadata both for flit and for setuptools. Thus, if we are going to go back to setuptools, I would be in favor of removing any dependency on flit (sorry, @takluyver ).

There is also some stuff in the docs configuration that has flit-related stuff. I think it's just the requirements.txt. If you could update that as well in this PR that would be awesome.

@jdfreder
Copy link
Contributor Author

jdfreder commented Jan 6, 2016

I don't recommend having both flit and setuptools packaging in the same repo - there's no particular conflict, but it means you'll be duplicating information and risking it getting out of sync.

@takluyver maybe we could make flit capable of parsing requirements.txt type files ? i.e. in the flit ini you could have a syntax like requires = ./requirements.txt. Or a step further, make it capable of parsing setup.py files?

@ellisonbg
Copy link
Contributor

I think this is important enough that we need to make a project wide
decision about our usage of flit. Right now, today, I can't do regular
development and deployment of nbgrader and that is causing me pain. I would
like us to revert to the original setup.py (distutils/setuptools) until
there is a project wide consensus about if/how we want to use flit.

On Wed, Jan 6, 2016 at 1:38 PM, Jonathan Frederic notifications@github.com
wrote:

I don't recommend having both flit and setuptools packaging in the same
repo - there's no particular conflict, but it means you'll be duplicating
information and risking it getting out of sync.

@takluyver https://github.com/takluyver maybe we could make flit
capable of parsing requirements.txt type files ? i.e. in the flit ini you
could have a syntax like requires = ./requirements.txt. Or a step
further, make it capable of parsing setup.py files?


Reply to this email directly or view it on GitHub
#430 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@takluyver
Copy link
Member

I went back and forth on whether requirements should be a separate file or a field in flit.ini. I'm still open to be convinced of supporting a separate file, but the bar is rather higher now that the integrated requirements is already supported, because adding another way goes against 'one obvious way to do it'.

I'm definitely not going to make it try to parse setup.py files, because that's an imperative format, so you can't reliably do anything without running it. If someone wants to make a way for setup.py to parse the declarative flit.ini files, they're welcome to do so - see d2to1 for something simialr.

@ellisonbg
Copy link
Contributor

I would love to have a purely declarative file that setup.py could read and
use for its data that drives distutils and setuptools. The declarative
format is part of what is so extremely nice about npm's package.json.

On Wed, Jan 6, 2016 at 1:43 PM, Thomas Kluyver notifications@github.com
wrote:

I went back and forth on whether requirements should be a separate file or
a field in flit.ini. I'm still open to be convinced of supporting a
separate file, but the bar is rather higher now that the integrated
requirements is already supported, because adding another way goes against
'one obvious way to do it'.

I'm definitely not going to make it try to parse setup.py files, because
that's an imperative format, so you can't reliably do anything without
running it. If someone wants to make a way for setup.py to parse the
declarative flit.ini files, they're welcome to do so - see d2to1
https://pypi.python.org/pypi/d2to1 for something simialr.


Reply to this email directly or view it on GitHub
#430 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@jdfreder
Copy link
Contributor Author

jdfreder commented Jan 7, 2016

It looks like you need to add back in the nbgrader script as well for this to work.

✔️

There is also some stuff in the docs configuration that has flit-related stuff. I think it's just the requirements.txt. If you could update that as well in this PR that would be awesome.

✔️

@jdfreder jdfreder force-pushed the setuppy branch 6 times, most recently from ebbfb10 to c6ba0d8 Compare January 7, 2016 01:42
@Carreau
Copy link
Member

Carreau commented Jan 7, 2016

One other way to do that is to have travis publish --pre wheels on PyPI.
Then you can just use the --pre, or even have custom repo that host wheels.

Min have one:
https://github.com/minrk/travis-wheels/tree/master/wheelhouse

That would make install faster. You can still target branches, tags hashes of repo.

We could even have a jupyter/wheels repo, with dev build of all the things.

Thoughts.

@Carreau Carreau removed the bugfix label Jan 7, 2016
cmd = 'python setup.py install'

# install
cmd = 'pip install -r dev-requirements.txt -e .'
Copy link
Member

Choose a reason for hiding this comment

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

The if statement that's there needs to be there, because the the docs don't seem to build correctly if its a symlinked install. Can you have it do pip install -r dev-requirements.txt . if it's docs and otherwise install it with -e?

@jhamrick
Copy link
Member

jhamrick commented Jan 8, 2016

I left one comment in the invoke script, otherwise this looks good. The tests are failing currently—probably because of a change in JupyterHub, I think—but I won't be able to look at that until after quals. So unless there are objections (@ellisonbg ?), I'll go ahead and merge this once my comment is addressed even though it will cause the tests to fail on master.

@jhamrick
Copy link
Member

jhamrick commented Jan 8, 2016

Oh, I guess one last thing—it would be awesome if you could update the development installation docs so that they instruct people to use pip (with the dev-requirements.txt) and remove the reference to flit: https://nbgrader.readthedocs.org/en/latest/contributor_guide/installation_developer.html

and remove flit from developer install documents.
@jdfreder
Copy link
Contributor Author

jdfreder commented Jan 8, 2016

@jhamrick last commit should address your comments, thanks!

@jhamrick
Copy link
Member

jhamrick commented Jan 8, 2016

Thanks!

jhamrick added a commit that referenced this pull request Jan 8, 2016
Reintroduce flit-less setup.py
@jhamrick jhamrick merged commit 30f461e into jupyter:master Jan 8, 2016
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.

Can pip install from github
5 participants