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

Use setuptools in the Python builder #288

Closed
wants to merge 13 commits into from

Conversation

bryanwweber
Copy link
Member

Resolves #283

@speth
Copy link
Member

speth commented Jul 22, 2015

I'm starting to remember some of the things I dislike about setuptools. Buildbot is reporting the following error when running scons install:

TEST FAILED: /var/lib/buildbot/slaves/cantera1/cantera-rls-ubuntu1/build/stage//usr/local/lib/python3.4/site-packages/ does NOT support .pth files
error: bad install directory or PYTHONPATH

You are attempting to install a package to a directory that is not
on PYTHONPATH and which Python does not read ".pth" files from.  The
installation directory you specified (via --install-dir, --prefix, or
the distutils default setting) was:

    /var/lib/buildbot/slaves/cantera1/cantera-rls-ubuntu1/build/stage//usr/local/lib/python3.4/site-packages/

and your PYTHONPATH environment variable currently contains:

    ''

Here are some of your options for correcting the problem:

* You can choose a different installation directory, i.e., one that is
  on PYTHONPATH or supports .pth files

* You can add the installation directory to the PYTHONPATH environment
  variable.  (It must then also be on PYTHONPATH whenever you run
  Python and want to use the package(s) you are installing.)

* You can set up the installation directory to support ".pth" files by
  using one of the approaches described here:

  https://pythonhosted.org/setuptools/easy_install.html#custom-installation-locations

Please make the appropriate changes for your system and try again.
scons: *** [interfaces/cython/dummy3] Error 1

@bryanwweber
Copy link
Member Author

Right, so what should we do about that? On one hand, it might eliminate some user-board posts if people are forced to set everything up properly before they install (install to a proper directory or add the proper directories to PYTHONPATH). I can confirm that the installation works for the USER folder on Ubuntu and with the MSI's on Windows. On the other hand, this behavior is really annoying and having to hack in a fix just to get the buildbot to work doesn't seem useful.

The most direct hack would be to override setuptools.command.easy_install.check_site_dir to force it to do nothing, but then if anything in setuptools changes, it breaks...

@speth
Copy link
Member

speth commented Jul 22, 2015

Well, we know the target installation directory in SCons, right? So I think we could just append that to the PYTHONPATH before running the setup.py install command. This isn't just a problem with the buildbot. It's a problem with any installation where a path is explicitly set for the Python module. This will probably also break building the Debian/Ubuntu packages, since those rely on installing to a stage directory which then gets rolled up into the .deb file, but that stage directory is of course never actually on the PYTHONPATH.

I don't think there's anything improper about installing a module to a location not on the PYTHONPATH, despite what setuptools thinks. Sure, you need to add it to the PYTHONPATH before you can import the module, but that could be handled any number of ways.

@bryanwweber
Copy link
Member Author

OK, I think we only need to add to PYTHONPATH if the --prefix is set. If --prefix is not set, the user is either installing in the default /usr/local, which setuptools should be fine with, or into their --user directory, which again, setuptools should have no problem . I pushed a commit to this branch that should set the proper PYTHONPATH, but there might be an easier way to determine the correct directory. I'm not totally familiar with all the variables available in the env, so if there's a better way to do this, please let me know. I don't think the pythonX_module_loc variable will be available, since that relies on the installed-files.txt which isn't generated until after the install. I tested this and it worked when I set the prefix to be /home/bryan/test as long as the lib/pythonX.Y/site-packages directory existed. It worked for both the minimal and full interfaces.

BTW, when/how does the buildbot run?

@speth
Copy link
Member

speth commented Jul 23, 2015

Yes, I agree that the logic with respect to the --prefix and --user options is correct. However, the path is not correct in a couple cases:

  • On Windows, the path is just ...Lib\site-packages\ (no pythonx.y directory)
  • We need to use '$python%s_prefix' % ver (and move the definition of ver up) to get the correct prefix for each Python version (which might be different, especially on Windows).

The buildbot runs on commits to the Github master branch, and to my local master branch. I've been meaning to move the latter set of builds to follow a separate "testing" repository on Github so we (and not just I) can get it to run tests without having to push commits to the active repository.

@bryanwweber
Copy link
Member Author

On Windows, something screwy happens when the prefix is specified into setuptools. The install fails with the similar error message (wrong directory), but the attempted installation directory is missing the first slash, i.e., it shows C:test\\Lib\\site-packages even though the prefix and PYTHONPATH are set to C:/test and C:/test\\Lib\\site-packages, respectively. It seems like everything works on Linux though.

Actually, in reading the docs related to this: https://pythonhosted.org/setuptools/easy_install.html#custom-installation-locations, it seems like the better option would be to set the PYTHONUSERBASE directory equal to the '$python%s_prefix' % ver and set the extra to --user instead. This way, there's no need to check the OS. In this case, the files are installed to

python%s_prefix/PythonXY/site-packages/Cantera-2.2.0-pyX.Y.egg  # Windows
python%s_prefix/lib/pythonX.Y/site-packages/Cantera-2.2.0-pyX.Y.egg  # Linux (Ubuntu)

The disadvantage is that setuptools will install the package as an egg, so the egg directory (instead of the site-packages) has to be added to PYTHONPATH so that the package can be used. This is automagically done in the setup_cantera script by the variable substitution.

This seems like a lot of machinations for a really simple feature, i.e., executable scripts on Windows. Maybe it would be easier to just write a .bat script for Windows and go back to distutils. OTOH, I think its true that having a binary (i.e., egg) distribution would allow uploading to PyPI (so pip would work, or at least, easy_install would) and would make creating a conda build recipe easier (I think).

I just pushed some code that implements the PYTHONUSERBASE method. BTW, if we decide to merge this, I'd like to rewrite the commits on this branch to have a cleaner commit history, probably by cherry-picking the correct (and maybe rebased) commits to a new branch.

@bryanwweber
Copy link
Member Author

Of course, I just noticed that on Windows using the PYTHONUSERBASE this way doesn't install the Scripts directory anywhere useful, it actually puts them in interfaces/cython/<prefix>/Scripts, where <prefix> is the python_prefix minus the drive, i.e., python_prefix=C:/testing, <prefix>=testing. I have no idea why this happens, or even, where the Scripts directory should go if the user specifies the python_prefix.

That said, does anyone actually use the scons install option on Windows? I've only ever used the scons msi option, even when I intend to install the package on the same computer. I suppose you could specify the python_prefix as C:/Python27 (or C:/Anaconda) and it might work, but it seems like this wouldn't work (even when using distutils) if you wanted to install into a directory that didn't contain the Lib and Scripts directory.

FWIW, specifying the USER option as the python%s_prefix on Windows (as opposed to an explicity directory) installs the scripts into %APPDATA%/Python/Scripts

@speth
Copy link
Member

speth commented Jul 24, 2015

I would like to avoid installing as an .egg at all costs. It causes all kinds of issues. The biggest one is that once a package has been installed that way once, i.e. installing Cantera to ~/.local/lib/python2.7/site-packages/ the path manipulations done by the easy-install.pth file there make it impossible to use a different version of Cantera by specifying PYTHONPATH since the other version gets inserted to the head of the path. This means you can't try things out in the development build by running PYTHONPATH=build/python2 ipython from the root of the source folder, which is something I do all the time. I think the --single-version-externally-managed flag will do this.

I guess the scripts should be installed to $inst_bindir in any case, right?

I've thought before about trying to get binaries distributed on PyPI, but I'm not sure how feasible that is. As far as I can tell, the new binary "wheel" format only really works for Windows, and don't know if that's any better than the MSI installers we already provide.

Yes, if and when this is ready to merge, some rebasing and squashing is definitely warranted. The approach I've been taking for merging pull requests is to ignore the "merge pull request" button, and instead use the link for "command line instructions" to get the "patch" URL, so you can then do:

wget https://github.com/Cantera/cantera/pull/288.patch
git am 288.patch

And then do whatever fixups / interactive rebasing you like before pushing the changes back.

@bryanwweber
Copy link
Member Author

OK, the --single-version-externally-managed option prevents installation into the *.egg directory and the creation of the easy-install.pth files. So that was easy enough...

However, I cannot figure out how setuptools determines the path to install the Windows console_scripts. Something isn't expanded properly and the installer prints Installing %s script to C:testing\Python\Scripts which means all the console_scripts end up in the interfaces/cython/testing/Python/Scripts directory since the slash is missing between C: and testing. All of the other Installing... log output shows the correct path except the console_scripts. If I could figure this out, the scripts could go in $inst_bindir, or in the default Scripts directory, I don't think it really matters when the user is controlling the prefix, since they'll have to add it to their %PATH% anyways.

I guess I'll try to create a really simple test package and try to figure it out more directly. Then I could also ask about it on StackOverflow...

@bryanwweber
Copy link
Member Author

OK so it seems like this is a bug in setuptools (or at least, I consider it a bug in setuptools, see http://stackoverflow.com/q/31629398/2449192). Fortunately, the workaround is really simple, its just to use os.path.normpath on the path that gets passed in and used for the PYTHONUSERBASE. With these changes, the install should be fixed. I also went ahead and updated the uninstaller to make sure it removes all the files that get installed. If this builds and installs properly, I'll do the rebase and either open a new PR or force push to this branch and update the commits.

@bryanwweber
Copy link
Member Author

@speth can you please check that I added the right license classifier to setup.py.in? Also, I think it would make sense to add the license somewhere in the docs, both the actual license text and a generic description of what it allows people to do.

@bryanwweber
Copy link
Member Author

Also, regarding the installing into the .egg directory, it's possible to specify which version of the package should be imported at import-time by using pkg_resources, as detailed in this SO post: http://stackoverflow.com/a/5266042/2449192

For example,

import pkg_resources
pkg_resources.require('FooPackage==1.2')
import FooPackage

would import that version of FooPackage. The reason I mention this is because I suspect that creating a Conda build recipe will be easier if Cantera is packaged up as an Egg. However, this is something that 1) I need to a do a lot more research on and 2) Should be discussed on the User's ("Dev") list before its implemented/merged.

@speth
Copy link
Member

speth commented Jul 31, 2015

Yes, I believe that is the correct license classifier, given that it doesn't seem to distinguish between the different flavors of BSD license.

What are the "Programming Language" classifiers supposed to mean? Are they an indicator if implementation language or languages the package can be used from?

If you want an Egg for the Conda recipe, we can just add a SCons option to do that and leave the default behavior as it is.

I think doing git push --force on this branch makes more sense than creating a new PR.

@bryanwweber
Copy link
Member Author

What are the "Programming Language" classifiers supposed to mean? Are they an indicator if implementation language or languages the package can be used from?

I'm not sure... The complete list is here: http://pypi.python.org/pypi?:action=list_classifiers. I poked around on PyPI and it seems like most people choose the classifiers that are associated with the project in some way, whether by being written (partially) in that language, or just being related to that language.

FWIW, I added the classifiers mostly for our internal use, on the principle that "documentation is better than no documentation." We could probably delete them though, if they're wrong/unnecessary, on the principle that "incorrect documentation is worse than no documentation."

@speth
Copy link
Member

speth commented Jul 31, 2015

The buildbot (http://gir.mit.edu:8010/waterfall) now additionally follows the branch testing branch, which you should feel free to force-push to to trigger new builds on the builders named cantera-gh-testing-*.

There is an issue with the OS X build, which gives the error:

error: can't combine user with prefix, exec_prefix/home, or install_(plat)base

Which is apparently a problem with Homebrew Python (see Homebrew/homebrew-python#187) that apparently has a workaround also setting an empty value of --prefix when using --user.

Add functions to ck2cti, ctml_writer, and a new file __main__.py to
mixmaster to be the entry_points locations that setuptools scripts
will call.
Replace distutils with setuptools in the Cython and python_minimal
interfaces. Add console_scripts option to generate OS specific
scripts to run ck2cti, mixmaster, and ctml_writer
Conform to 4 space tabs. Add classifiers about Cantera. Add long
description.
Remove script files that are obsoleted by console_scripts from
setuptools. Remove installation of the script modules from
SConstruct.
Fix Python installers so that when a prefix directory is specified on
the command line, setuptools doesn't throw an error. The setuptools
documentation at [1] prefers setting PYTHONUSERBASE rather than
PYTHONPATH. Use normpath to avoid bugs in setuptools on Windows [2].
Specify an empty "--prefix" if the compiler is clang to fix a bug
with Homebrew Python on Mac OSX [3].

[1]: https://pythonhosted.org/setuptools/easy_install.html#custom-installation-locations
[2]: http://stackoverflow.com/q/31629398
[3]: https://github.com/Homebrew/homebrew-python/issues/187
@bryanwweber
Copy link
Member Author

OK I think I fixed the error, although I'm not sure that the check I do is appropriate. There is currently a build error with "ubuntu-1", but it exists on the master branch build too, so I don't think its anything that I did. I rebased these commits against the current master (6574117) so a merge should be pretty easy. Thanks for all your support/code review!

@speth
Copy link
Member

speth commented Aug 3, 2015

The errors with the Ubuntu build were my fault (since fixed). I changed the test for OS X to look for env['OS'] == 'Darwin' instead of checking for Clang, since Clang is a perfectly reasonable compiler on Linux as well.

Merged as 0b5a46f...2334410.

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.

2 participants