ericmandel / pyds9

Python connection to SAOimage DS9 via XPA
76 stars 41 forks source link

Install #28

Closed montefra closed 8 years ago

montefra commented 8 years ago

This PR tries to build libxpa using the machinery provided by astropy_helper.

Now it copies all the *.{c,h} files into a temporary directory and begins compiling, but it fails: I'll work on the errors and try to figure out in the next days what options are missing.

If it gets too hard, we can revert to the original implementation in the setup.py file.

ericmandel commented 8 years ago

Can I help? What sort of errors are you getting?

montefra commented 8 years ago

I'm going to work on this on the underground beginning of next week and I will compare the compiler options I get now with the ones I get when doing ./configure.

I'll let you know soon. Thank for the help.

ericmandel commented 8 years ago

OK, let me know ... remember to check setup.py for switches that get set programmatically in the script.

montefra commented 8 years ago

I've managed now to create all the .o files. Running python setup.py build it fails when creating libxpa.so.

The full log is here

ericmandel commented 8 years ago

/data/Codes/Gitrepos/pyds9/cextern/xpa/xpaget.c:17: multiple definition of `usage' build/temp.linux-x86_64-3.4/cextern/xpa/xpaaccess.o:/data/Codes/Gitrepos/pyds9/cextern/xpa/xpaaccess.c:19: first defined here

I think you are trying to add the program C files (xpaget.c, xpaset.c, xpaaccess.c, etc) to the libxpa.so shared library. They have to be treated separately.

montefra commented 8 years ago

Ok, thanks for digging it out.

Actually I'm not trying to do it, astropy does it, somehow. To compile c extensions using the astropy machinery, I use a couple of functions defined in pyds9/setup_package.py.

One (get_external_libraries) returns the names of the library to build, the other (get_extensions) returns an Extension object that contains the information in theory needed to build libxpa: the directories with the h files and the list of all the .c files to compile, plus a bunch of compile options.

astropyhelpers adds a number of other compile options, compile all the .c files into .o (storeded into a build/tmp_/cextern/xpa directory) and then try to make a libxpa.so file combining all the .o files.

It seems that most of the "first defined" errors are coming from {c,r,s}tests.c files. Do we need them in libxpa? If I exclude them from the building I get less errors (I've updated the gist)

Any suggestion? If not I can try to contact either astropy people or stackoverflow, unless @cdeil can give us some hint.

ericmandel commented 8 years ago

The {c,r,s}test should not be part of an ordinary build, so by all means leave them out.

Is setup_package.py standard? Because it makes the assumption that all .c files are part of the library and that is incorrect in this case:

glob(os.path.join(os.path.dirname(__file__), 'src', '*.c')))

This probably should run off a list of files, not take a glob. What options do you have for fixing this within the standards of python? Is your option manually to move files out of the xpa directory? (I don't want to rework xpa itself.)

montefra commented 8 years ago

the setup_package.py file is astropy standard to build c/c++ extensions. It doesn't make that assumption, it's just that I copied it from some astropy. In the following commits I've modified it to our needs.

The get_extension function returns an Extension class, which contains, among other things, a list of c/c++ files to compile, the path to the directory/ies containing the header files and a list of arguments. The list of c files is taken and each one is compiled to an object file, which is saved into build/tmp*/cextern/xpa. When all the object files are created, it tries to merge them into libxpa.so.

No c file is moved and for sure you don't need to modify xpa itself.

Back to the list of c files: we can write the name of all the files to compile by hand, if necessary. glob simply returns a list of files matching the given pattern. In my last commit I get all the c files and exclude the test ones.

I hope that his clarify your questions.

Now I have a question myself: do we need any executable create by xpa, or is everything contained in libxpa.so?

ericmandel commented 8 years ago

This is all good. I think you can safely make the source file list by hand, since I doubt xpa will ever change. I certainly do not intend to extend it. But note that BASE_OBJS and XT_OBJS in Makefile.in are what you need for the pyds9 library, if you want to extract the list from that file.

You do need to build the xpans program for pyds9. This is the xpa name server, which is started up automatically by ds9, and which connects the "ds9" target to the actual socket address that ds9 is listening on.

montefra commented 8 years ago

@ericmandel success number 1: I can create libxpa now!! :smile: Thank for your help. Now there are issues with importing the library, since is called something like libxpa.cpython-34m.so: I'll try to figure out how astropy does it.

Then I'll try to create the xpans executable. I'll keep you updated.

ericmandel commented 8 years ago

Nice work! Let me know if I can help.

embray commented 8 years ago

I commented on this in the Astropy-dev mailing list, but instead of messing around with ctypes in xpa.py you may wish to create your wrappers as a Python extension module, which would have the xpa library compiled into it (unless linking to a system version of libxpa).

embray commented 8 years ago

You might also consider wrapping the functions you use from libxpa using Cython, which is often easier than writing the wrappers by hand.

ericmandel commented 8 years ago

@embray Yes, thanks for weighing in ... see #21 (based on discussions in #2), where we decided to experiment with Cython. I just haven't had time to work on it.

montefra commented 8 years ago

@embray : Thank you very much for the tip. I'll work on it the next days

@ericmandel : I suggest that I finish this work. I think/hope that this is the last step to make pyds9 install and work using the astropy infrastructure.

Once I'm done we can see how to proceed.

ericmandel commented 8 years ago

@montefra thanks for offering to do this work! I know I have no time for it until July or August, so it will be a real help if you can fit it into your schedule. Thanks again.

montefra commented 8 years ago

Thanks to @embray suggestions and git grep I think that I found the way to build xpans using pre/post hooks. I've added a couple of placeholders and I'll play with it as soon as I can.

@ericmandel: I would like to have the installation sorted out within the current framework first. I have no experience with Cython and I will love to take the opportunity to learn it, but I want to do it knowing that pyds9 builds and run first. Anyway I'm doing now will be useful also with the cython way

ericmandel commented 8 years ago

@montefra Yes, I agree that you should deal with one variable at a time! I do appreciate your willingness to work on pyds9.

montefra commented 8 years ago

Finally I can compile and install libxpa and xpans. And pyds9 works again :+1:. I'm trying to figure out how to uninstall it cleanly.

When installing with pip, the installed files are recorded and the record is put in the installed egg directory. It then uses the list to uninstall the files. For some obscure reason, pip installs xpans, but does not record it. I've tried to run python setup.py install --record record-file.txt and xpans appears in the list. Bah! But now I'm of on vacation. I hope to have some time to work on this in the next 3 weeks.

Happy Easter

ericmandel commented 8 years ago

Wish I could help, but .... Happy Easter to you!

montefra commented 8 years ago

@ericmandel : Finally I managed to get the installation to properly work. This was harder than expected [and I can't say that I worked very hard on it]. To get all up and running I had to manually run configure into cextern/xpa and to build the xpans executable.

Can you please try it on your computer(s)? I've tested it on a Linux machine on python2 and 3 and it works. I suggest to do it in a clean virtualenv or conda env that then you can through away.

pip install git+https://github.com/montefra/pyds9.git@install#egg=pyds9
python -c 'from pyds9 import pyds9; pyds9.test()'

If this works, I'll merge this into the devel branch and then try to merge it into master.

Then we can start to discuss about travis and documentation.

ericmandel commented 8 years ago

@montefra You're so close! I got an error during the build because you are making the totally understandable mistake of looking for Tcl/Tk and then trusting that, if found, it can be used. This is often not the case, because Tcl/Tk has its own special config setup. So on my Mac, newly updated to El Capitan:

   gcc -fno-strict-aliasing -I/Users/eric/miniconda2/envs/pyds9/include -arch x86_64 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -o build/lib.macosx-10.6-x86_64-2.7/pyds9/xpans build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/xpans.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/acl.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/client.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/clipboard.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/command.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/find.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/port.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/remote.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/tcl.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/tclloop.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/tcp.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/timedconn.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/word.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/xalloc.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/xlaunch.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/xpa.o build/temp.macosx-10.6-x86_64-2.7/cextern/xpa/xpaio.o -DHAVE_CONFIG_H -Wno-declaration-after-statement -Wno-unused-variable -Wno-parentheses -Wno-uninitialized -Wno-format -Wno-strict-prototypes -Wno-unused -Wno-comments -Wno-switch -Wno-strict-aliasing -Wno-return-type -Wno-address -Wno-unused-result
    Undefined symbols for architecture x86_64:
      "_Tcl_CreateFileHandler", referenced from:
          _XPATclAddInput in tclloop.o
          _XPATclAddOneInput in tclloop.o
          _XPATclDelOneInput in tclloop.o
          _XPATclEnableOneInput in tclloop.o
          _XPATclDisableOneInput in tclloop.o

and a lot of other Tcl unresolved externals because the Tcl libraries were not installed in a standard place.

I recommend that you run your configure with the --without-tcl switch:

configure --without-tcl

and not attempt to build Tcl ...

montefra commented 8 years ago

@ericmandel : thank you for checking and reporting. Now I run configure --withoug-tcl and among other things I see this line: checking for Tcl configuration... skipping Tcl configuration.

Can you please try again and let me know?

ericmandel commented 8 years ago

@montefra Yes! Now I get this:

(pyds9)bash-3.2$ pip install --upgrade git+https://github.com/montefra/pyds9.git@install#egg=pyds9
Collecting pyds9 from git+https://github.com/montefra/pyds9.git@install#egg=pyds9
  Cloning https://github.com/montefra/pyds9.git (to install) to /private/var/folders/hl/0rxttnz48xldqq006s7f7t980000gp/T/pip-build-N3ncCC/pyds9
Requirement already up-to-date: astropy in /Users/eric/miniconda2/envs/pyds9/lib/python2.7/site-packages (from pyds9)
Requirement already up-to-date: numpy in /Users/eric/miniconda2/envs/pyds9/lib/python2.7/site-packages (from pyds9)
Installing collected packages: pyds9
  Found existing installation: pyds9 1.7
    DEPRECATION: Uninstalling a distutils installed project (pyds9) has been deprecated and will be removed in a future version. This is due to the fact that uninstalling a distutils project will only partially uninstall the project.
    Uninstalling pyds9-1.7:
      Successfully uninstalled pyds9-1.7
  Running setup.py install for pyds9 ... done
Successfully installed pyds9-1.8

Running:

python -c 'import pyds9; pyds9.test()'

gives me 3 DS9 window containing valid images and contours. I also played around a bit manually and things seem to work as expected.

Very nicely done!

montefra commented 8 years ago

Great! It's time to go to bed and dream a party! Thank you again for checking.