CastXML / pygccxml

pygccxml is a specialized XML reader that reads the output from CastXML or GCCXML. It provides a simple framework to navigate C++ declarations, using Python classes.
Boost Software License 1.0
131 stars 45 forks source link

CastXML is default even when only GCC-XML is present #65

Closed cls closed 7 years ago

cls commented 7 years ago

On a system with GCC-XML but not CastXML, this fails:

pygccxml.parser.parse(files)
gccxml_cc1plus: error: unrecognized command line option "-x"
gccxml_cc1plus: error: unrecognized command line option "--castxml-cc-gnu"
gccxml_cc1plus: error: unrecognized command line option "--castxml-gccxml"
gccxml_cc1plus: error: c++: No such file or directory

But by doing a little dance with pygccxml.utils (which is labelled 'internal' in the docs), this succeeds:

xml_generator_path, xml_generator = pygccxml.utils.find_xml_generator()
config = pygccxml.parser.xml_generator_configuration_t(xml_generator=xml_generator)
pygccxml.parser.parse(files, config=config)

So pygccxml finds the right configuration, it just doesn't sensibly default to it.

This is a regression in 1.8; it does not occur in 1.7.6.

iMichka commented 7 years ago

Hi

Thanks for reporting.

Since version 1.8.0, the xml_generator is CastXML by default. I rephrased the changelog entry. I realise that I was not clear enough about that change, which is my fault. See 625b21661543ce7452e414996e7e6f17a0cb9527 for the change.

pygccxml still supports GCCXML, but is is highly recommended to used CastXML. See the FAQ: http://pygccxml.readthedocs.io/en/develop/faq.html#gccxml-vs-castxml gccxml support will completely be removed in version 2.0.0, so depending on how fast this goes, probably at the end of 2017/beginning 2018.

Using pygccxml.utils.find_xml_generator() is the way to go, see also the first example: http://pygccxml.readthedocs.io/en/master/examples/parsing/example.html It is even recommended to pass the path to your xml generator directly to your config by yourself. find_xml_generator() is quite naive: if GCCXML or CastXML is not in your path, or you have multiple versions installed, it will have trouble guessing what generator to use.

Of course, this could be seen as a regression. But on the other side people having just CastXML installed were confused by the same error, but the other way round. Not sure it is worth adding more code for supporting GCCXML, as it will be removed anyway.

I will have a look at the doc entry about the utils module, it should not be marked as 'internal', that's probably an error.

cls commented 7 years ago

If you've passed no configuration for either the XML generator or its path, how does it make sense to default to anything other than what would be detected by pygccxml.utils.find_xml_generator()? If the recommendation is to always call that function, why not just call it if no path is supplied? Clearly it is better if the user supplies the path themselves, but if they don't, why complicate things by requiring that the user do this little dance, when you wouldn't even need find_xml_generator to be anything but internal if you just defaulted to the values it comes up with anyway? People with only CastXML wouldn't even notice, since find_xml_generator favours CastXML now, and if it isn't in your path then it won't work anyway.

So as far as I can tell, calling find_xml_generator can be nothing but a formality. Also, find_xml_generator pretty much duplicates pygccxml.parser.xml_generator_configuration_t.raise_on_wrong_settings, so by merging these two you should end up with less code, not more. I would be happy to write a patch, if it is likely to be accepted.

iMichka commented 7 years ago

Right, I get it now. I completely forgot about the code in raise_on_wrong_settings. In fact, I did not write it initially (I picked up this project only 2 years ago), and I did not double check that part of the code lately (just made some cosmetic changes). It was also not tested at all, which is annoying too.

My idea (since version 1.6 at least) was that you have to set the xml_generator and xml_generator_path by yourself and inject these values. The config itself should not have any detection logic. It decouples the part of the gccxml/castxml path setup from the rest of the code. I think we agree on this. The old code made sense when there was no other xml generator.

I just created a 1.8.2 branch. I cleaned up the config and added a test. I hope the current version will be less surprising for the users. You can review the change here: 1bc25ba3691440c76cab090509a2565172d71727

I will release 1.8.2 in a few days I think.

I would have reviewed a patch with pleasure, maybe I should have let you write it, but as this goes on a special release branch before merging back to master, I preferred to do it myself.

Thanks also again for reporting. Feedback is always welcome. I am trying to not break too many things by changing the API, but it is difficult. pygccxml was written years ago and really needs a cleanup. And I am working on the addition of new features, as CastXML is evolving too. Don't hesitate to tell me if things are unclear or if you find bugs.

cls commented 7 years ago

That works for me. It might be nice if it did take a stab at finding the generator on its own, but I can see the argument why it shouldn't. The error is much clearer now anyway. And I'm happy that I didn't have to write it! :)

iMichka commented 7 years ago

I just tagged the 1.8.2 release and pushed it to pypi. Closing here.