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

Use sphinx.ext.linkcode for more precise source code links (backport #11851) #12146

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Apr 5, 2024

Summary

This addresses Qiskit/documentation#517 by switching out the sphinx.ext.viewcode for the sphinx.ext.linkcode extension which allows our GitHub links in the documentation to be linked to the specific lines of code, not just the file. This was tested successfully in Qiskit/qiskit_sphinx_theme#589 so now we want to implement it in the qiskit, runtime, and provider repos.

Example links generated in this PR build:

The API generation script at qiskit/documentation already knows how to handle sphinx.ext.linkcode.

Determining the GitHub branch

We need to detect when to use main vs stable branches like stable/1.0, particularly for stable docs builds with GitHub Actions that get used by qiskit/documentation to deploy the docs. PR and local builds are less critical because the docs don't get published and are only for previews by code authors.

We do this with GitHub Actions environment variables. The docs workflow sets the env variable QISKIT_DOCS_GITHUB_BRANCH_NAME read by Sphinx in conf.py. You can see the workflow correctly passing the value to Sphinx in this example run.

Examples of the workflow logic:

❯ GITHUB_REF_NAME="1.0.1rc1" bash test.sh  # tag
Using branch 'stable/1.0' for GitHub source code links
❯ GITHUB_REF_NAME="stable/0.2" bash test.sh     # branch build   
Using branch 'stable/0.2' for GitHub source code links
❯ GITHUB_BASE_REF=main bash test.sh  # PR
Using branch 'main' for GitHub source code links

PR builds use main

PR builds and the merge queue use Azure Pipelines. To keep this infrastructure simpler, we simply default to main in PR builds, rather than adding Azure support.

This means the URL will go to the wrong branch in docs previews for PRs against stable branches, although the stable branch build will work correctly. I don't expect this to matter much because PR builds are solely for docs previews; I'm skeptical code authors will be opening up the [source] link to double check the line numbers are exactly correct, since that line number calculation is 100% automated.


This is an automatic backport of pull request #11851 done by Mergify.

* switched from sphinx.ext.viewcode to sphinx.ext.linkcode

* removed extra line

* Add section header for source code links

Co-authored-by: Eric Arellano <[email protected]>

* removed docstring

Co-authored-by: Eric Arellano <[email protected]>

* update return string

Co-authored-by: Eric Arellano <[email protected]>

* added back blank line

* Added a method to determine the GitHub branch

Co-authored-by: Eric Arellano <[email protected]>

* add blank line

Co-authored-by: Eric Arellano <[email protected]>

* remove print statement

Co-authored-by: Eric Arellano <[email protected]>

* Try to fix error for contextlib file

We got this stacktrace:

Traceback (most recent call last):
  File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit
    results.append(listener.handler(self.app, *args))
  File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read
    uri = resolve_target(domain, info)
  File "/home/vsts/work/1/s/docs/conf.py", line 216, in linkcode_resolve
    file_name = PurePath(full_file_name).relative_to(repo_root)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/pathlib.py", line 908, in relative_to
    raise ValueError("{!r} does not start with {!r}"
ValueError: '/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/contextlib.py' does not start with '/home/vsts/work/1/s'

We shouldn't even attempt to generate a link for the file contextlib.py

* Try to fix error for Jenkins run #20240221.52

New build failed with this stacktrace:
Traceback (most recent call last):
  File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit
    results.append(listener.handler(self.app, *args))
  File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read
    uri = resolve_target(domain, info)
  File "/home/vsts/work/1/s/docs/conf.py", line 215, in linkcode_resolve
    full_file_name = inspect.getsourcefile(obj)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/inspect.py", line 696, in getsourcefile
    filename = getfile(object)
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/inspect.py", line 665, in getfile
    raise TypeError('{!r} is a built-in class'.format(object))
TypeError: <class 'builtins.CustomClassical'> is a built-in class

So I added a condition that the obj should not be a built-in

* Check that qiskit in module name sooner

* moved valid code object verification earlier

* added try except statement to getattr call

* added extra try/except block

* Also support Azure Pipelines

* removed unused import

* Revert Azure support to keep things simple

* added extra "/" to final URL

* Move GitHub branch logic to GitHub Action

* switched to importlib and removed redundant block of code

* Apply suggestions from code review

Co-authored-by: Eric Arellano <[email protected]>

* added back spaces

* Clarify docs_deploy GitHub logic

1. Remove misleading PR conditional. This worfklow doesn't even run in PRs. It was bad copy-pasta from the runtime repo
2. Clarify why we point tag builds to their stable branch name

* Use pathlib for relativizing file name

* Fix relative_to() path

* Remove tox prefix

---------

Co-authored-by: Eric Arellano <[email protected]>
(cherry picked from commit 97788f0)
@mergify mergify bot requested a review from a team as a code owner April 5, 2024 16:47
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@github-actions github-actions bot added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo labels Apr 5, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8573060548

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 15 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.3%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.03%
crates/qasm2/src/lex.rs 2 92.62%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 8559808978: -0.01%
Covered Lines: 58898
Relevant Lines: 65955

💛 - Coveralls

@jakelishman jakelishman added this pull request to the merge queue Apr 5, 2024
Merged via the queue into stable/1.0 with commit 0625a76 Apr 5, 2024
14 checks passed
@mergify mergify bot deleted the mergify/bp/stable/1.0/pr-11851 branch April 5, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo documentation Something is not clear or an error documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants