VO - #46

It looks like astropy/io/vo/src is missing?

i686-apple-darwin10-gcc-4.2.1: astropy/io/vo/src/iterparse.c: No such file or directory
i686-apple-darwin10-gcc-4.2.1: no input files
error: command '/usr/bin/gcc-4.2' failed with exit status 1

Yes, indeed. I was foiled by the "*.c" in .gitignore again ;)

Ok, I get further, but I cannot build this branch on MacOS 10.6 with Python 2.7:

/usr/bin/gcc-4.2 -fno-strict-aliasing -fno-common -dynamic -pipe -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -DHAVE_EXPAT_CONFIG_H=1 -DBYTEORDER=1234 -DHAVE_UNISTD_H -Iastropy/io/vo/src -Icextern/expat/lib -I/opt/local/Library/Frameworks/Python.framework/Versions/2.7/include/python2.7 -c astropy/io/vo/src/iterparse.c -o build/temp.macosx-10.6-x86_64-2.7/astropy/io/vo/src/iterparse.o
astropy/io/vo/src/iterparse.c:1506: warning: ‘_state’ defined but not used
/usr/bin/gcc-4.2 -fno-strict-aliasing -fno-common -dynamic -pipe -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -DHAVE_EXPAT_CONFIG_H=1 -DBYTEORDER=1234 -DHAVE_UNISTD_H -Iastropy/io/vo/src -Icextern/expat/lib -I/opt/local/Library/Frameworks/Python.framework/Versions/2.7/include/python2.7 -c cextern/expat/lib/xmlparse.c -o build/temp.macosx-10.6-x86_64-2.7/cextern/expat/lib/xmlparse.o
cextern/expat/lib/xmlparse.c:20:26: error: expat_config.h: No such file or directory
cextern/expat/lib/xmlparse.c:81:2: error: #error memmove does not exist on this platform, nor is a substitute available
cextern/expat/lib/xmlparse.c: In function ‘XML_Parse’:
cextern/expat/lib/xmlparse.c:1491: warning: enumeration value ‘XML_FINISHED’ not handled in switch
error: command '/usr/bin/gcc-4.2' failed with exit status 1

@astrofrog: Sorry. Again, it was a missing file. I just tested with a fresh clone and it seems to be working now, and passing all tests.

It installs fine now and all tests run successfully.

The documentation has some mentions of the networking capability (e.g. Note The network functionality in the conesearch, ssa and image modules is experimental and subject to change.) - maybe this should be removed for now?

Minor: on the main documentation page, the headings for the WCS and VO stuff aren't consistent (astropy.wcs Documentation vs vo.table documentation. Also, are you planning on including the vo history?

Good catch. Yes. I think we should just remove those for now.

Will fix the documentation headers to match wcs.

I wasn't planning on including history -- after all the moves etc. it's not clear it would be directly useful. The SVN history will live on indefinitely as a resource in a bind.

About the included expat: I didn't entirely understand your last comment there - you're saying we don't want to let the user build against the system expat... is this just for the performance reasons? I think we should still allow the system expat (or even the python builtin expat) to be used if the user desires, as long as we make it clear that it will be slower. I can certainly imagine systems where something goes wrong with the astropy expat builds, but the user says "screw it, I'm fine with a 15% slowdown because I don't want to bother figuring out how to compile astropy." Is there any reason why these can't be left as options? (For now, feel free to just put it in as a constant or something in setup_package.py - we can update it after we have the extension cfg system working).

Oh, also, just so we're clear, the cextern expat is only statically linked to iterparser, right? So an expat library file is never actually generated?

I should have been more clear about the expat. Expat has a number of #defines that change how it is built, and different Linux distros build in different ways. The one that matters here is whether it returns utf-8 or ucs4 to the caller. To handle the system expat, we'd have to handle and test both cases, and also do extra work if it is built for utf-8. That's where the 15% comes in. vo.table also has a fallback to pure Python standard library for the XML parsing, but that is about 3x slower. So, IMO, it's preferable to build our own expat -- it's very simple to build -- and use that. And we have a pure Python fallback which could be selected using our forthcoming build configuration system in case building expat as part of astropy is a problem for some users (it seems very unlikely as it's very portable C code).

To answer your question -- no standalone expat library is built, it simply builds the 4 required C files as part of the iterparse extension.

Ah, that all makes sense re: expat. It's a bit annoying that we have to include it, but I see you point for why it's useful. We definitely want the option of a fallback to the python builtin version, but we can deal with that later (as you say).

@eteq: Just to clarify -- the pure Python fallback already exists and is tested as part of the unit tests. The only thing missing is an option for a user to choose not to build the C extension.

I couldn't get github to show me all of the diffs, I think because this pull request has so many files in it. So the stuff above is inline comments that github isn't showing you the context for.

One other thing: when I run the tests on this, it modifies the file astropy/io/vo/tests/data/test_through_binary.xml, which is under git version control. I don't think we want the tests modifying anything under version control - maybe either have git forget it, or if it has a role in the test before the test is run, perhaps write to a temporary file?

Good catch about test_through_binary.xml. That should be in the tmp dir and was never intended to be checked in.

Does anyone know a way to make the diff's visible? I want to be able to look at the diffs for some of the files at the end of the list, but they're all hidden (I think because the page would be too big, otherwise)

Yeah -- it's annoying. You can of course look at the diff offline, but then there's no code review ;) I could take expat out of here for now, but then we'll have a branch that doesn't actually "work".

@eteq: I think this latest set of commits addresses everything you raised.

So is this good to go? I don't see anything else that needs changes that can't be post-merge - @astrofrom and @iguananaut?

Oh, and @mdboom - when it does get merged, can you create issues for the things we mentioned as "todo after merge"? Thanks.

@eteq: it seems fine to me (disclaimer: I haven't checked it all line by line).

Yeah, I think it's the sort of thing where it's going to be best to go ahead and get things merged in order to start tweaking them more. In particular we need to work on centralizing some of the Py3K hacks.

Ok. Merging now... Will add some issues for various TODO list items as @eteq suggests.