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

DRLParser fails while building the master branch #77

Open
sztomi opened this issue Dec 5, 2016 · 22 comments
Open

DRLParser fails while building the master branch #77

sztomi opened this issue Dec 5, 2016 · 22 comments

Comments

@sztomi
Copy link

sztomi commented Dec 5, 2016

After setting the flag to download clang and having installed ncurses5-compat-libs, I'm observing an exception in DRLParser. Relevant parts of the output:

siplasplas reflection parser
============================

==> Scanning search directories:
  -> /home/sztomi/src/siplasplas/include/siplasplas/fswatch
  -> /home/sztomi/src/siplasplas/include/siplasplas/signals
  -> /home/sztomi/src/siplasplas/include/siplasplas/reflection/static
  -> /home/sztomi/src/siplasplas/include/siplasplas/typeerasure
  -> /home/sztomi/src/siplasplas/include/siplasplas/reflection/common
  -> /home/sztomi/src/siplasplas/include/siplasplas/utility
  -> /home/sztomi/src/siplasplas/include/siplasplas/logger
  -> /home/sztomi/src/siplasplas/include/siplasplas/constexpr

==> Processing 69 files:
[  1%] (outdated) include/siplasplas/constexpr/algorithm.hpp
[  2%] (outdated) include/siplasplas/constexpr/arrayview.hpp
[  4%] (outdated) include/siplasplas/constexpr/assert.hpp
Traceback (most recent call last):
  File "/home/sztomi/src/siplasplas/src/reflection/parser/DRLParser", line 238, in <module>
    App().run()
  File "/home/sztomi/src/siplasplas/src/reflection/parser/DRLParser", line 235, in run
    self.compiler.run()
  File "/home/sztomi/src/siplasplas/src/reflection/parser/DRLParser", line 86, in run
    self.process()
  File "/home/sztomi/src/siplasplas/src/reflection/parser/DRLParser", line 72, in process
    tu.process(self.verbose)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/translationunitprocessor.py", line 49, in process
    self.translation_unit = TranslationUnit(self.clang_tu.cursor, self.filePath)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/translationunit.py", line 55, in __init__
    self.root = Namespace.global_namespace(self)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/namespaces.py", line 32, in global_namespace
    return Namespace.create_node(cursor = translation_unit.cursor, translation_unit = translation_unit, file = translation_unit.file)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 125, in create_node
    nodeClass.initialize_children(node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 160, in initialize_children
    child = nodeClass.create_child(cursor = c, parent = node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 115, in create_child
    return class_.create_node(**kwargs)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 125, in create_node
    nodeClass.initialize_children(node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 160, in initialize_children
    child = nodeClass.create_child(cursor = c, parent = node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 115, in create_child
    return class_.create_node(**kwargs)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 125, in create_node
    nodeClass.initialize_children(node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 160, in initialize_children
    child = nodeClass.create_child(cursor = c, parent = node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 115, in create_child
    return class_.create_node(**kwargs)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 125, in create_node
    nodeClass.initialize_children(node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 160, in initialize_children
    child = nodeClass.create_child(cursor = c, parent = node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 115, in create_child
    return class_.create_node(**kwargs)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 125, in create_node
    nodeClass.initialize_children(node)
  File "/home/sztomi/src/siplasplas/src/reflection/parser/ast/node.py", line 159, in initialize_children
    if c.kind in mapping:
  File "/usr/lib/python2.7/site-packages/clang/cindex.py", line 1271, in kind
    return CursorKind.from_id(self._kind_id)
  File "/usr/lib/python2.7/site-packages/clang/cindex.py", line 555, in from_id
    raise ValueError,'Unknown template argument kind %d' % id
ValueError: Unknown template argument kind 602
make[2]: *** [src/fswatch/CMakeFiles/siplasplas-fswatch_drlparser.dir/build.make:58: siplasplas-fswatch_drlparser] Error 1
make[1]: *** [CMakeFiles/Makefile2:1722: src/fswatch/CMakeFiles/siplasplas-fswatch_drlparser.dir/all] Error 2
make: *** [Makefile:161: all] Error 2

I ran:

mkdir build && cd build
cmake ..
make
@sztomi
Copy link
Author

sztomi commented Dec 5, 2016

Adding

CursorKind.STATIC_ASSERT = CursorKind(602)

To /usr/lib/python2.7/site-packages/clang/cindex.py solves the problem, but obviously it's not a great idea. Maybe it would make sense to ship a patched cindex.py with siplasplas until the patch is upstream.

@Manu343726
Copy link
Owner

Maybe the long-term solution to all this problems is to provide our own versioned versions of cindex.py, one for each tested clang version. This may include some hotfixes like yours. In general, a way to avoid relying in external unstable releases if possible.

Note: In the future, I would like to deprecate the python DRLParser and rewrite everything using C++ and the libclang C API

@sztomi
Copy link
Author

sztomi commented Dec 6, 2016

I would be interested in contributing to that.

@Manu343726
Copy link
Owner

Cool, feel free to send PRs to the master branch ;)

What I would do:

  • Add a cindex/ folder in src/reflection/parser/
  • Add versioned cindex.py files there, named cindex-x.y.py, where x.y is the clang version (3.4, 3.8, etc)
  • Remove clang python package dependency from cmake/libclang.cmake
  • Change python scripts in src/reflection/parser/ to import cindex API from cindex/cindex.x.y.py instead of the system package

@sztomi
Copy link
Author

sztomi commented Dec 6, 2016

I think it's enough to have just one cindex.py because in my experience the incompatibilities stem from certain things not being exposed (like the CursorKind above), and not things like changing APIs (after all, libclang aims to be a stable, backwards-compatible library). That implies that there is no need to change the scripts that import it if it's in the correct directory.
By the way, I mainly meant contributing to the C++ rewrite of DRLparser, but I can help with this as well :)

@Manu343726
Copy link
Owner

By the way, I mainly meant contributing to the C++ rewrite of DRLparser, but I can help with this as well

OH. That's even better! I haven't done before due to lack of time and, frankly, being a bit lazy...
If we are going to continue this way, @foonathan 's standardese is a great reference.

@Manu343726
Copy link
Owner

Manu343726 commented Dec 6, 2016

One of the things I have been thinking for months was to directly take Standardese backend and adapt the generation part to generate the siplasplas reflection metadata. This way we:

  • Depend on a widely tested high-quality libclang based backend. (Which also supports much more features than my DRLParser does)
  • Contribute to standardese. Standardese bugfixes help us and we help Standardese

@Manu343726
Copy link
Owner

Manu343726 commented Dec 6, 2016

I think it's enough to have just one cindex.py

So, we could maintain a cindex.py file compatible with all the clang versions we support (Something we must define and fix first)

@sztomi
Copy link
Author

sztomi commented Dec 6, 2016

One of the things I have been thinking for months was to directly take Standardese backend

Good idea, I'll take a look.

Something we must define and fix first

I think the minimal version is the first clang version that supports the C++ standard that siplasplas wants to support. If it's C++14, then if I'm not mistaken it's 3.7(?)

@Manu343726
Copy link
Owner

I think the minimal version is the first clang version that supports the C++ standard that siplasplas wants to support. If it's C++14, then if I'm not mistaken it's 3.7(?)

I have been trying to maintain C++11 compatibility at least in the static reflection API, so the DRLParser generated code and the reflection API should be C++11 compatible. (I think there are some minor issues, last time I tried it worked with a GCC 4.8 with some warning about generic lambdas).

@sztomi
Copy link
Author

sztomi commented Dec 6, 2016

Since C++ standards are mostly backwards-compatible, it's not a bad idea to opt for a minimal version of clang that supports a newer standard (i.e. to be able to parse the widest set of possible inputs). The generated code should not depend on this at all. ANd libclang supports the -std flag.

@sztomi
Copy link
Author

sztomi commented Dec 7, 2016

Do you want to rewrite all of DRLParser in C++ or only replace the actual parsing with Standardese? I think the latter would be fairly easy if we made a Cython wrapper around Standardese. The jinja template would probably have to be changed a bit. This could also be used as an intermediate step towards a full rewrite.

@Manu343726
Copy link
Owner

I want to replace the whole DRLParser tool to get rid of python dependencies. Also, writing DRLPaser in C++ makes it easier to write a C++ API to invoke it programatically (Which I would like to have for runtime c++ compilation)

@sztomi
Copy link
Author

sztomi commented Dec 7, 2016

I'm looking at a replacement for Jinja and found this: https://github.com/hughperkins/Jinja2CppLight

At a first glance, it misses macros that is feature that siplasplas templates seem to heavily depend on. Are there other features missing?

This is another approach: https://github.com/hughperkins/luacpptemplater which might be easier to implement macros with. Maybe @hughperkins has some input?

@hughperkins
Copy link

hughperkins commented Dec 7, 2016 via email

@sztomi
Copy link
Author

sztomi commented Dec 8, 2016

I can see where you are coming from. I agree that the lua option is fairly good for this, what do you think, @Manu343726 ? (There is also a lua package on conan). This is probably the direction with the least resistance, because changing the template syntax is way less work than adding the missing features to jinjacpplight.

@Manu343726
Copy link
Owner

I looked at Jinja2CppLight previously when searching jinja2 replacements, but didn't know about luacpptemplater. I think it is perfect for our use case.

The only thing I'm worried about is conan.io support of this dependency, since I want to completely switch to conan for thirdparty management (See #78). But the project looks simple to port to conan ;)

@sztomi
Copy link
Author

sztomi commented Dec 8, 2016

@hughperkins is it possible to define a lua function in the template as a "macro"? What I mean:

{% function something() %}
  hey, render this
{% endfunction %}

{% something() %}
{% something() %}

would output

  hey, render this
  hey, render this

@hughperkins
Copy link

hughperkins commented Dec 8, 2016

is it possible to define a lua function in the template as a "macro"?

Interesting question. I added a test at hughperkins/luacpptemplater@38cc752 to check this point. This test passes.

Note that the lua templater code idea and core implementation came from John Nachtimwald https://john.nachtimwald.com/2014/08/06/using-lua-as-a-templating-engine/ . I'm not sure why I didnt point this out in the readme before, but I've added it now hughperkins/luacpptemplater@9611b32

@sztomi
Copy link
Author

sztomi commented Dec 8, 2016

That's great, thank you!

@Manu343726
Copy link
Owner

@hughperkins thank you for the support!
@sztomi I'm currently working on migrating dependencies to conan, I hope I can start with the DRLParser rewrite by next week. Anyway, thanks a lot for the feedback, I'm really looking forward to review your PRs ;)

@sztomi
Copy link
Author

sztomi commented Dec 8, 2016

In the meantime I started to freshen up luacpptemplater a bit. I'll take a stab at making a conan package as well. https://github.com/sztomi/luacpptemplater

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants