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

Patch setup #181

Merged
merged 8 commits into from Aug 7, 2015

Conversation

Projects
None yet
3 participants
@benjello
Copy link
Collaborator

commented Jul 20, 2015

First tentative to start discussion
I am not able to test it on Windows and I am not very happy about the hack
Comments very welcome

INSTALL Outdated

Install cython

aptitude install cython

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 20, 2015

Member

I'm more into the apt-get camp. I never used aptitude, and from what I read about it, it does not seems necessarily worse (nor better) but AFAIK, apt-get is still the more used/known tool of the two, so would you mind changing that?

INSTALL Outdated

Install cx_freeze cx_freeze

aptitude install mercurial

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 20, 2015

Member

Yuk! Why not install cx_freeze via pip from pypi directly? And why do you install it at all since it does not seem to be used in setup.py?

This comment has been minimized.

Copy link
@benjello

benjello Jul 21, 2015

Author Collaborator

On my debian box, I cannot install cx_freeze directly (bug) that is why I need the little trick in the setup.py of cx_freeze
If it is not needed, I can completely remove it but you may need it from the windows side.

This comment has been minimized.

Copy link
@benjello

benjello Jul 21, 2015

Author Collaborator

Tell me if you think I should remove cx_freeze completely.

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 21, 2015

Member

Yes, I think it makes sense to only mention it in the optional dependencies (like it is now). It seems unlikely people other than me will use cx_freeze (and those who would use it would probably be savvy enough to not need the instructions). But now that I think of it, why aren't you using conda anyway?

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 21, 2015

Member

I mean: remove it from the install instructions, not from the setup.py where I do need it (but it could be made optional, which would be a good thing).

This comment has been minimized.

Copy link
@benjello

benjello Jul 21, 2015

Author Collaborator

No conda on debian and I tend to stick to the debian way of doing things ;-)
I will keep cx_freeze as an option.

This comment has been minimized.

Copy link
@benjello

benjello Jul 21, 2015

Author Collaborator

No debian package ;-)

setup.py Outdated
from cx_Freeze import setup, Executable
else:
from setuptools import setup
from setuptools.extension import Extension

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 20, 2015

Member

I can never remember what is the actual situation between distutils & setuptools: which one is the currently recommended one, etc. but it seems odd to use something different on Linux than everything else.

This comment has been minimized.

Copy link
@benjello

benjello Jul 21, 2015

Author Collaborator

I think setuptools is the new norm, at least it is what more exprienced dev I work with on other project use.

setup.py Outdated
),
)
else:
setup(name="liam2",

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 20, 2015

Member

Ouch, this is ugly. Why not have something like an "other_kwargs" dict which would store extra arguments and would contain {'executables': [Executable("main.py")]} on non-linux platforms and then get rid of the if block? I also doubt this is the right way to go, see my general remarks.

.gitignore Outdated
*.so
*.o
*.egg-info
.spyderproject
doc/usersguide/build/
src/build
src/*.c

This comment has been minimized.

Copy link
@gdementen

gdementen Jul 20, 2015

Member

plz remove those two lines (src/*)

@gdementen

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

Thanks a lot for taking care of this. I have been wanting to improve the Linux experience for ages but never took the time to do it. BTW, if you could also fix the examples to run on Linux out of the box, that would be super great ;-)

So far, the moves look good. The setup.py changes less so. The ideal situation would be if you made cx_freeze entirely optional (whatever the platform), but allow it to work on linux too.

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2015

I started tweaking the tests too. More to come. Thank you for fast review.

@gdementen

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2015

I have a proposal for the tests but unfortunately I hacked on the same branch. Lets deal with the setup first then we will move to the tests.

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 5, 2015

@gdementen are you okay with these modifications ? Should I proceed with the tests on the same PR ?

INSTALL Outdated

apt-get install python2.7 python-numpy python-tables python-numexpr python-yaml

Eventually install the optional dependencies:

This comment has been minimized.

Copy link
@gdementen

gdementen Aug 5, 2015

Member

eventually does not mean "éventuellement" in French... ;-)

)
other_kwargs = dict()
if Executable is not None:
other_kwargs['executables'] = [Executable("main.py")]

This comment has been minimized.

Copy link
@gdementen

gdementen Aug 5, 2015

Member

Did you test this? main.py is in liam2 directory, right?

This comment has been minimized.

Copy link
@benjello

benjello Aug 5, 2015

Author Collaborator

Not on windows and I do pip install on my debian machine. Do you have time to give it a try ?

This comment has been minimized.

Copy link
@gdementen

gdementen Aug 6, 2015

Member

Ok, I will try to do this today. But my question was whether you tested cx_freeze on Linux (I assume it should work on Windows too if it works on Linux).

@gdementen

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

@benjello sorry for the slow reaction. It looks good to me IF it actually works, which I fear it does not (see inline comment). For the tests, I would prefer another PR, if that's not too much trouble.

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 5, 2015

@gdementen : tests and new files hierarchy are a bit intricated. Since it is not that urgent, let's go through this part of the PR and then move to the tests

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2015

I tested the install this way by reinstalling cx_freeze
aptitude install mercurial
hg clone https://bitbucket.org/anthony_tuininga/cx_freeze
cd cx_freeze
In setup.py remplace
if not vars.get("Py_ENABLE_SHARED", 0):
by
if True:
python setup.py build
python setup.py develop --user

But I didn't tested more extensively since I didn't know what to expect ... and since cx_freeze cannot work out of the box on my debian machine and the install via pip does fit my needs.
But I certainly could write some tests for that if you explain what you are expecting.

@landscape-bot

This comment has been minimized.

Copy link

commented Aug 6, 2015

Code Health
Repository health increased by 0.15% when pulling 26b4c82 on benjello:patch_setup into 64c1419 on liam2:master.

@gdementen

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

Sorry, I didn't get to this today. I wanted to finish something else. I hope to be able to squeeze it into tomorrow's schedule (just before my holidays).

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2015

Thx

Le jeu. 6 août 2015 21:33, Gaëtan de Menten notifications@github.com a
écrit :

Sorry, I didn't get to this today. I wanted to finish something else. I
hope to be able to squeeze it into tomorrow's schedule (just before my
holidays).


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

gdementen added a commit that referenced this pull request Aug 7, 2015

Merge pull request #181 from benjello/patch_setup
s/src/liam2/ and support install via pip

@gdementen gdementen merged commit 89b2e03 into liam2:master Aug 7, 2015

@gdementen

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

I just tested it. As expected, it did break .exe generation (and the release script). After quite a bit of trial & error I managed to fix the .exe generation (*). I will commit those fixes now, and fix the remaining of the release script later (when I do the next release ;-)).

For some reason, 99% of cxfreeze-driven setup.py examples I could find (and ALL the official ones) have the setup.py in the same directory than the "main" script.

Thanks a lot for this PR and pushing me a bit to do this. I have been postponing this for too long (possibly because of the cx-freeze examples "issue" I just mentioned, I don't remember).

@benjello

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2015

Thanks for going through this.
Should I proceed with a PR about using nose for testing ?

@gdementen

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

nose will not work out of the box on our current functional & examples tests, which are 99% of our current tests. If you want to make them work that way (via a small wrapper), that's fine with me, but try to not be too nose-specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.