manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
165 stars 50 forks source link

Allow python-config for Python 2 and Python 3 #159

Closed lgarrison closed 6 years ago

lgarrison commented 6 years ago

Fixes virtualenv install, which only links python-config instead of python3-config (closes #156)

I was able to reproduce the installation problem reported in #156 and confirm that this fixes it.

astropy-bot[bot] commented 6 years ago

Hi there @lgarrison :wave: - thanks for the pull request! I'm just a friendly :robot: that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part :smiley:.

Everything looks good from my point of view! :+1:

If there are any issues with this message, please report them here.

lgarrison commented 6 years ago

This works on Travis for all but Python 2.6, which doesn't have the sysconfig module, so sysconfig.get_path('scripts') fails. distutils.sysconfig provides similar functionality on Python 2.6, but the canonical way to get the bin/ dir via distutils.sysconfig.get_config_var('BINDIR') does not give the correct answer inside a virtualenv (points to the original location).

So we could use the hacky distutils.sysconfig.EXEC_PREFIX + '/bin', or just drop Python 2.6 support...

manodeep commented 6 years ago

Thanks for sorting this out. I think we should completely remove the python-config dependence; all the flags we need are stored under sysconfig

One reason why I am proposing such a change is that I just looked at my python situation on my laptop and looks like /usr/bin/python-config is being picked up, even though I am using the python from anaconda3/bin. I also know of people that use python as an alias...

Separately, I think it is fine to drop python2.6 support but we should make plans for future releases and document them, perhaps on the wiki. Such a drop for python2.6 should probably be done in a minor version update 2.2 (with as-yet un-released 2.1 for DDsmu*)

lgarrison commented 6 years ago

What path do you get if you run python -c "import sysconfig; print(sysconfig.get_path('scripts'));"? That's the solution that's implemented in this PR; the global python-config is never called.

manodeep commented 6 years ago

I get /Users/msinha/anaconda3/bin and there are a bunch of python*-config executables within there.

I would still say, when python3 is being used, we check for python3-config first before checking python-config

lgarrison commented 6 years ago

Makes sense, although hopefully it won't make a difference either way. I've pushed that and it seems to have passed Travis.

I also went ahead and dropped Python 2.6 support and edited the docs to reflect this.

manodeep commented 6 years ago

Since we are potentially introducing breaking changes, should we merge this into a different minor version?

lgarrison commented 6 years ago

Probably! I figured we'd work out release versioning over Skype.

lgarrison commented 6 years ago

We're going to wait until v2.1 is released in master before merging this in.

manodeep commented 6 years ago

New Corrfunc 2.1.0 is now live on PyPI

manodeep commented 6 years ago

@lgarrison Is it okay to delete the venv_fix branch now?

lgarrison commented 6 years ago

I think so? Unless we wanted to ask the original issue poster to verify the fix out of master.

On Fri, Aug 17, 2018, 6:50 PM Manodeep Sinha notifications@github.com wrote:

@lgarrison https://github.com/lgarrison Is it okay to delete the venv_fix branch now?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/pull/159#issuecomment-414006824, or mute the thread https://github.com/notifications/unsubscribe-auth/AFYPy1e867_SIqnIFMSyTYoU8ACou_tNks5uR0i9gaJpZM4U2rTH .

manodeep commented 6 years ago

I think the resolution was suggested by @rainwoodman - so should be okay. Deleting the branch; we can always un-delete if required.

Related, should we add a python_requires into the setup.py file?

lgarrison commented 6 years ago

Yes, I think so. I'll do that in my capture_stderr branch so we can CI it in one go.