Skip to content

Add Documentation for Execution Results Handling in Clang-Repl #65650

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 12, 2023
Merged

Add Documentation for Execution Results Handling in Clang-Repl #65650

merged 1 commit into from
Oct 12, 2023

Conversation

Krishna-13-cyber
Copy link
Contributor

[clang-repl] Add Documentation for Execution Results Handling.

This patch adds documentation for execution results handling in Clang-REPL with
the below features:

  • Automatic Printf feature
  • Value Synthesis feature
  • Pretty Printing feature

Continuing this work https://reviews.llvm.org/D156858 with this PR. I am issuing this patch on behalf of Saqib.

Reviewers:
@vgvassilev
@junaire

@Krishna-13-cyber Krishna-13-cyber requested a review from a team as a code owner September 7, 2023 17:47
@vgvassilev vgvassilev requested a review from gribozavr September 7, 2023 17:48
@vgvassilev
Copy link
Contributor

@gribozavr, after this PR we will need to have dot for the sphinx documentation for the bots. Could you install it on the bots? Eg, https://lab.llvm.org/buildbot/#/builders/92/builds/49520

Further details: https://discourse.llvm.org/t/any-tool-for-creating-editable-diagrams-in-llvm-clang-repl-documentation-similar-to-mermaid-ingithub/72729/2

@Krishna-13-cyber
Copy link
Contributor Author

Ping.

p4vook
p4vook reviewed Oct 3, 2023
View reviewed changes
vgvassilev
vgvassilev reviewed Oct 3, 2023
View reviewed changes
@vgvassilev
Copy link
Contributor

@Krishna-13-cyber, can you rebase this PR?

@gribozavr ping.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Oct 3, 2023
@Krishna-13-cyber Krishna-13-cyber force-pushed the ClangRepl-ExecResults branch 2 times, most recently from f7037ce to 001c8fe Compare October 4, 2023 09:37
@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited
Loading

✅ With the latest revision this PR passed the Python code formatter.

vgvassilev
vgvassilev approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's move forward.

@vgvassilev vgvassilev merged commit b9b8fc4 into llvm:main Oct 12, 2023
@AaronBallman
Copy link
Collaborator

This appears to have broken the clang-tools-extra Sphinx bot: https://lab.llvm.org/buildbot/#/builders/115/builds/54901

CMake Error at /home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/src/clang/docs/CMakeLists.txt:110 (message):
  Cannot find DOT

@vgvassilev
Copy link
Contributor

@AaronBallman, we need to install dot/doxygen on this machine as Sphinx requires graphviz. The PR has a link to the discussion and we could not find the bot owners.

@AaronBallman
Copy link
Collaborator

@AaronBallman, we need to install dot/doxygen on this machine as Sphinx requires graphviz. The PR has a link to the discussion and we could not find the bot owners.

Ouch, we've struggled recently to get the attention of the sphinx bot owners. :-(

The publication bot is still working: https://lab.llvm.org/buildbot/#/builders/89/builds/49495 and seems to be generating the docs for clang-tools-extra:

42.046 [42/24/363] Generating html Sphinx documentation for clang-tools into "/home/buildbot/as-worker-4/publish-sphinx-docs/build/tools/clang/tools/extra/docs/html"

so I don't think we need to revert the changes here (the Clang and LLVM sphinx bots are also broken because we needed the machine to be modified by the bot owners, so there is already precedence). However, you should be aware that the sphinx publish bot does not send out email notifications when the documentation is broken -- you need to manually keep track of the state of things. We're trying to improve this situation, so I don't expect that to be a forever problem, but it's something to keep in mind. CC @LegalizeAdulthood @carlosgalvezp @njames93 for awareness as other frequent reviewers of clang-tools-extra stuff.

@nikic nikic mentioned this pull request Oct 13, 2023
Closed
@nikic
Copy link
Contributor

nikic commented Oct 13, 2023

This also breaks the clang standalone build. You need to include(config-ix) in order to use llvm_find_program.

@nikic
Copy link
Contributor

nikic commented Oct 13, 2023

Actually no, that's not possible as config-ix modules are explicitly private to their subproject and should not be used outside them. I'm going to revert this change due to this layering violation.

nikic added a commit that referenced this pull request Oct 13, 2023
@nikic
Revert "Add Documentation for Execution Results Handling in Clang-Repl (
ac32d7b 
#65650)"

This reverts commit b9b8fc4.

This uses a function defined in LLVM's config-ix inside clang.
config-ix is a non-exported cmake module, so this is a layering
violation.
junaire
junaire reviewed Oct 13, 2023
View reviewed changes
@vgvassilev
Copy link
Contributor

vgvassilev commented Oct 13, 2023 •
edited
Loading

Actually no, that's not possible as config-ix modules are explicitly private to their subproject and should not be used outside them. I'm going to revert this change due to this layering violation.

@nikic, can you suggest a particular fix? We saw this approach was used elsewhere as well…

@nikic
Copy link
Contributor

nikic commented Oct 13, 2023

You probably should just use find_program instead of llvm_find_program. It looks like llvm_find_program is used in just a single place, while we have very wide direct use of find_program, so I'm not sure why it is a thing in the first place.

@vgvassilev
Copy link
Contributor

@Krishna-13-cyber, could you implement the suggestion and we can try to recommit the code.

vgvassilev
vgvassilev reviewed Oct 14, 2023
View reviewed changes
clang/docs/CMakeLists.txt
@@ -103,6 +103,13 @@ function (gen_rst_file_from_td output_file td_option source docs_targets)
endfunction()

if (LLVM_ENABLE_SPHINX)
llvm_find_program(dot)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krishna-13-cyber, could we drop the llvm_find_program(dot) and rely on handling the dot discovery from llvm. This is what we do a few lines above for the case of doxygen.

@madhur13490 madhur13490 mentioned this pull request Oct 20, 2023
Closed
vgvassilev added a commit that referenced this pull request Oct 24, 2023
@vgvassilev
Reland "Add Documentation for Execution Results Handling in Clang-Repl (
1f02743 
#65650)"

Original commit message: "

Add Documentation for Execution Results Handling in Clang-Repl (#65650)

    [clang-repl] Add Documentation for Execution Results Handling.

    This patch adds documentation for execution results handling in
    Clang-REPL with
    the below features:

    - Automatic Printf feature
    - Value Synthesis feature
    - Pretty Printing feature

    Continuing this work https://reviews.llvm.org/D156858 with this PR. I am
    issuing this patch on behalf of Saqib.

"

This reverts commit ac32d7b.

The new patch resolves the layering violation by simply not trying to look if
`dot` was installed. This is consistent with what we do other extensions.

After this patch we will need `sphinx.ext.graphviz` extension (part of the `dot`
package) which is available via anyway if we build the doxygen-based
documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronBallman AaronBallman AaronBallman left review comments

@vgvassilev vgvassilev vgvassilev approved these changes

@p4vook p4vook p4vook left review comments

@junaire junaire junaire left review comments

@gribozavr gribozavr Awaiting requested review from gribozavr

Assignees
No one assigned
Labels
clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Krishna-13-cyber @vgvassilev @AaronBallman @nikic @p4vook @junaire @llvmbot