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

Triggered assertions in parse_desc_internal #81

Closed
Naios opened this issue Jan 6, 2019 · 10 comments
Closed

Triggered assertions in parse_desc_internal #81

Naios opened this issue Jan 6, 2019 · 10 comments
Assignees

Comments

@Naios
Copy link

Naios commented Jan 6, 2019

Hey @mosra,

My environment is the following:
m.css: 9c97cf8
Doxygen: 1.8.14
Python: 3.6.1

I'm encountering two triggered assertions in parse_desc_internal when generating the documentation for Naios/continuable@20e8c7d :

diff --git a/m.css/doxygen/dox2html5.py b/m.css/doxygen/dox2html5.py
index 76857aa..3392789 100644
--- a/m.css/doxygen/dox2html5.py
+++ b/m.css/doxygen/dox2html5.py
@@ -1567,10 +1567,10 @@ def parse_desc_internal(state: State, element: ET.Element, immediate_parent: ET.
 
         # Sane behavior otherwise
         else:
-            assert not has_block_elements and paragraph_count <= 1
+            # this-> assert not has_block_elements and paragraph_count <= 1
 
             if paragraph_count == 1:
-                assert out.parsed.startswith('<p>') and out.parsed.endswith('</p>')
+                # this-> assert out.parsed.startswith('<p>') and out.parsed.endswith('</p>')
                 out.parsed = out.parsed[3:-4]
 
     # Strip superfluous <p> for simple elments (list items, parameter and

beside of design changes my m.css matches the one as of 9c97cf8 .
I could also open a seperate issue for each assertion if wished.

Thanks in advance.

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

Hi, thanks for the detailed report!

I can have a look later today and try to generate the project, however if you could run the script with --debug and upload here the Doxygen-generated XML file it fails on (the filename will get mentioned right above the stack trace), that could speed it up a bit ;)

This particular portion of the code is often hitting very nasty Doxygen featurebugs (as the # Sane behavior otherwise comment may suggest). I wonder what is it this time 😅

@mosra mosra self-assigned this Jan 6, 2019
@mosra
Copy link
Owner

mosra commented Jan 6, 2019

Oh, just realized: 1.8.15 fixed some of the stuff related to this (and based on that I made some changes in this function). Is there a chance you could test with 1.8.15, maybe it goes away?

Nevertheless, I want to stay compatible with 1:8.14, so this has to be fixed.

@Naios
Copy link
Author

Naios commented Jan 6, 2019

Thanks for your quick response, I'll try out 1.8.15 but I would prefer staying at 1.8.14 since 1.8.15 introduced some regressions (I can't remind which bugs it introduced but it prevented me from generating a project I was working on last year).

See the log and xml output attached as requested:
xml_and_log.zip

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

Thanks! Yeah, I got also quite some regressions, had to submit a few PRs for them. Is there a chance any of (doxygen/doxygen#6715, doxygen/doxygen#6714) affect you too?

By the way, looking at https://naios.github.io/continuable/ I see that search.js is not getting downloaded, even though I can see it in the gh-pages branch. Maybe GitHub doesn't like the Base85-encoded file ¯\_(ツ)_/¯. Can you try enabling M_SEARCH_DOWNLOAD_BINARY and uploading a binary file instead? It'll be also smaller, the text version is just to work around XHR restrictions when viewing docs locally.

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

Okay, looking at this, it boils down to a bit too much of the documentation recognized by doxygen as brief docs, causing problems visible for example here: https://naios.github.io/continuable/modules.html

As a workaround fix, can you add an empty line before this line here? That should make it put the rest into detailed docs and behave correctly.

I still need to fix the script so it doesn't blow up this way, however this is quite a pathological case that's hard to recover from. So I guess it'll just emit a loud warning (suggesting what you need to do) and ignore the whole docs.

@Naios
Copy link
Author

Naios commented Jan 6, 2019

So,1.8.15 triggers the assertions as well but the .html is generated instead on 1.8.14 which doesn't produce any output, although I also added the newline, thanks for the hint.
However the output is unusable but it seems that the generation went one step forward.

The search doesn't work on the current version even on localhost, so I don't think that setting M_SEARCH_DOWNLOAD_BINARY would make any difference right?

Because of the related issues:

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

So,1.8.15 triggers the assertions as well but the .html is generated instead on 1.8.14 which doesn't produce any output, although I also added the newline, thanks for the hint.

Um, not sure what you meant here 😅 I tried locally with current master (including the commit that added newline) and it works both on 1.8.15 and 1.8.14. With 1.8.15 it works even with the newline missing, for me at least. They fixed this exact thing (brief docs containing block elements) for 1.8.15, so that makes sense. (Though I have a slightly newer version of 1.8.15, containing fixes for the above and a bunch of other unrelated things.)

So, to be clear: the newline worked for you, and now I just need to handle the assertions more gracefully, right?

The search doesn't work on the current version even on localhost

image

The gh-pages branch was missing the search.js file 😉 -- I got confused because I thought the searchdata.js is missing, but that one is there. Not sure how that happened tho, the script should be copying that unless you disable the search globally. I failed to find the m.css-specific Doxyfile so I was not able to reproduce. Nevertheless, I would suggest enabling the binary search, it's slightly faster to download and process (even though you have just 12 kB of search data, so it is not that big difference).

Also found a few issues regarding parsing noexcept specifiers, will fix these (along with the ones reported in #79).

@Naios
Copy link
Author

Naios commented Jan 6, 2019

Ok, so the previous error I mentioned was due to a non existing favicon and can be ignored.

I managed to get m.css to work again so this behaviour is the same as on your machine now (including the search bar).
So it should be done by making the assertions taking the bad doxygen 1.8.14 output into account,
and allowing larger briefs in sections.

Thanks again for your help and the creation of m.css,
it really helps me to provide a helpful documentation for my project.

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

due to a non existing favicon

Saw that too, yeah, setting M_FAVICON = (empty) in m.css-specific Doxyfile should disable that. There is favicon-dark.png bundled with m.css (which also should get copied), but I'm not sure if that one fits your design.

and allowing larger briefs in sections

No, this was a bug of Doxygen < 1.8.15, the briefs should always be single-paragraph and the rest should go into detailed docs, shown later on the page. Allowing larger briefs would complicate styling a lot. I'll update the assertions and to make sure this doesn't happen again I'll try to test with 1.8.13 on the CI as well. Unfortunately not with 1.8.14, since the official binary release was broken and never properly fixed.

Thanks

Thanks for the appreciation and don't hesitate to open further issues if you see something that doesn't look right ;)

@mosra
Copy link
Owner

mosra commented Feb 23, 2019

So, finally:

  • with 990d436 and 540c41f the assertion should hopefully not be hit again on 1.8.14 with problematic input (nevertheless, now you already know how to avoid the assertion so all is good)
  • and with 38e0d62 the noexcept labels e.g. here should be recognized properly

Besides that, since quite a while ago the theme supports showing what to #include for given class / namespace / symbol (providing you have the files documented using the @file command), along with other niceties, so you might want to upgrade :)

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

No branches or pull requests

2 participants