Skip to content

[Docs] Update documentation for the new GitHub workflow #65162

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

Merged
merged 5 commits into from
Sep 1, 2023
Merged

[Docs] Update documentation for the new GitHub workflow #65162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ceaa087
[Doc] Update documentation for the new GitHub workflow
tru Sep 1, 2023
13e01d2
Code review updates.
tru Sep 1, 2023
45a213d
Another rephrase
tru Sep 1, 2023
1363885
Added reminders about running tests, and links to other relevant docu…
tru Sep 1, 2023
148e4f7
Add paragraph about keeping one pr == one commit
tru Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions llvm/docs/CodeReview.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,8 @@ author, for example, might no longer be an active contributor to the project.
What Tools Are Used for Code Review?
------------------------------------

Pre-commit code reviews are conducted on our web-based code-review tool (see
:doc:`Phabricator`). Post-commit reviews can be done on Phabricator, by email
on the relevant project's commit mailing list, on the project's development
list, or on the bug tracker.
Pre-commit code reviews are conducted on GitHub with Pull Requests. See
:ref:`GitHub <github-reviews>` documentation.

When Is an RFC Required?
------------------------
Expand Down Expand Up @@ -147,12 +145,6 @@ approved. If unsure, the reviewer should provide a qualified approval, (e.g.,
you are fairly certain that a particular community member will wish to review,
even if that person hasn't done so yet.

If you approve of the idea/concept of a patch but feel unqualified to approve,
another option (other than accepting the patch) is to simply *"Award Token"*
(right-hand side in Phabricator) to indicate support without indicating to
other reviewers that the patch has been accepted and reviewed in their
dashboard.

Note that, if a reviewer has requested a particular community member to review,
and after a week that community member has yet to respond, feel free to ping
the patch (which literally means submitting a comment on the patch with the
Expand Down
45 changes: 13 additions & 32 deletions llvm/docs/Contributing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,17 @@ in order to update the last commit with all pending changes.
the git integration can be run from
``clang/tools/clang-format/git-clang-format``.

We don't currently accept GitHub pull requests, and you'll need to send patches
via :ref:`Phabricator#phabricator-reviews <phabricator-reviews>`.
(We used to allow patches on the llvm-commits mailing list, but the mailing lists
have been deprecated.)
The LLVM project has migrated to GitHub Pull Requests as its review process.
We still have an active :ref:`Phabricator <phabricator-reviews>`
instance for the duration of the migration. If you want to contribute to LLVM
now, please use GitHub. For more information about the workflow of using GitHub
Pull Requests see our :ref:`GitHub <github-reviews>` documentation.

To make sure the right people see your patch, please select suitable reviewers
and add them to your patch when requesting a review. Suitable reviewers are the
code owner (see CODE_OWNERS.txt) and other people doing work in the area your
patch touches. If you are using Phabricator, add them to the `Reviewers` field
when creating a review and if you are using `llvm-commits`, add them to the CC of
your email.
patch touches. Github will normally suggest some reviewers based on rules or
people that have worked on the code before.

A reviewer may request changes or ask questions during the review. If you are
uncertain on how to provide test cases, documentation, etc., feel free to ask
Expand All @@ -110,8 +110,7 @@ access, please let people know during the review and someone should commit it
on your behalf.

If you have received no comments on your patch for a week, you can request a
review by 'ping'ing a patch by responding to the email thread containing the
patch, or the Phabricator review with "Ping." The common courtesy 'ping' rate
review by 'ping'ing the GitHub PR with "Ping." The common courtesy 'ping' rate
is once a week. Please remember that you are asking for valuable time from other
professional developers.

Expand All @@ -122,28 +121,9 @@ For more information on LLVM's code-review process, please see :doc:`CodeReview`
For developers to commit changes from Git
-----------------------------------------

Once a patch is reviewed, you should rebase it, re-test locally, and commit the
changes to LLVM's main branch. This is done using `git push` if you have the
required access rights. See `committing a change
<Phabricator.html#committing-a-change>`_ for Phabricator based commits or
`obtaining commit access <DeveloperPolicy.html#obtaining-commit-access>`_
for commit access.

Here is an example workflow using git. This workflow assumes you have an
accepted commit on the branch named `branch-with-change`.

.. code-block:: console

# Pull changes from the upstream main branch.
% git checkout main && git pull
# Rebase your change onto main.
% git rebase --onto main --root branch-with-change
# Rerun the appropriate tests if needed.
% ninja check-$whatever
# Check that the list of commits about to be pushed is correct.
% git log origin/main...HEAD --oneline
# Push to Github.
% git push origin HEAD:main
Copy link
Member

Choose a reason for hiding this comment

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

L165 below talks about a pre-push hook; but the diff removed above removes the recommended workflow about git push and the new Github doc recommends using gh pr merge.

Two thoughts:

  1. Does the pre-push hook still run if using the gh CLI? If not, we might need to clean that up.
  2. Can we link to the Github page's section on merging "Landing your change" from this page's "For developers to commit changes from Git" section?

Once a patch is reviewed, you can select the "Squash and merge" button in the
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was my impression that either "Squash and merge" or "Rebase and merge" are both acceptable workflows to use? If my impression is correct, maybe both options should be stated here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently only have squash and merge enabled. See my comment below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this means manual pushes are still acceptable, especially in the context of multi-commit sequences. That is probably worth stating explicitly somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the old workflow had "rebase it, re-test locally", but the new one has "click the merge button YOLO". This is fine in a world where the pre-commit testing is as good or better than what people are likely to do locally, but I'm not really sure we're there...

GitHub web interface. You might need to rebase your change before pushing
it to the repo.

LLVM currently has a linear-history policy, which means that merge commits are
not allowed. The `llvm-project` repo on github is configured to reject pushes
Expand Down Expand Up @@ -201,5 +181,6 @@ of LLVM's high-level design, as well as its internals:
.. _bug tracker: https://github.com/llvm/llvm-project/issues
.. _clang-format-diff.py: https://reviews.llvm.org/source/llvm-github/browse/main/clang/tools/clang-format/clang-format-diff.py
.. _git-clang-format: https://reviews.llvm.org/source/llvm-github/browse/main/clang/tools/clang-format/git-clang-format
.. _LLVM's Phabricator: https://reviews.llvm.org/
.. _LLVM's GitHub: https://github.com/llvm/llvm-project
.. _LLVM's Phabricator (deprecated): https://reviews.llvm.org/
.. _LLVM's Open Projects page: https://llvm.org/OpenProjects.html#what
13 changes: 8 additions & 5 deletions llvm/docs/DeveloperPolicy.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ quality.
Stay Informed
-------------

Developers should stay informed by reading the `LLVM Discourse forums`_.
Developers should stay informed by reading the `LLVM Discourse forums`_.
If you are doing anything more than just casual work on LLVM, it is suggested that you also
subscribe to the "commits" mailing list for the subproject you're interested in,
such as `llvm-commits
Expand Down Expand Up @@ -84,11 +84,8 @@ to read it as possible. As such, we recommend that you:
patches may not apply correctly if the underlying code changes between the
time the patch was created and the time it is applied.

#. Patches should be unified diffs with "infinite context" (i.e. using something
like `git diff -U999999 main`).

#. Once you have created your patch, create a
`Phabricator review <Phabricator.html#phabricator-request-review-web>`_ for
:ref:`GitHub Pull Request <github-reviews>` for
it (or commit it directly if applicable).

When submitting patches, please do not add confidentiality or non-disclosure
Expand Down Expand Up @@ -116,6 +113,12 @@ diagnostic from a warning to an error, switching important default behavior, or
any other potentially disruptive situation thought to be worth raising
awareness of. For such changes, the following should be done:

.. warning::

Phabricator is deprecated and will be switched to read-only mode in October
2023, for new code contributions use :ref:`GitHub Pull Requests <github-reviews>`.
This section contains old information that needs to be updated.

* When performing the code review for the change, please add any applicable
"vendors" group to the review for their awareness. The purpose of these
groups is to give vendors early notice that potentially disruptive changes
Expand Down
21 changes: 8 additions & 13 deletions llvm/docs/GettingInvolved.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ LLVM welcomes contributions of all kinds. To get started, please review the foll
CodeReview
SupportPolicy
SphinxQuickstartTemplate
Phabricator
HowToSubmitABug
BugLifeCycle
CodingStandards
Expand All @@ -38,10 +37,6 @@ LLVM welcomes contributions of all kinds. To get started, please review the foll
A template + tutorial for writing new Sphinx documentation. It is meant
to be read in source form.

:doc:`Phabricator`
Describes how to use the Phabricator code review tool hosted on
http://reviews.llvm.org/ and its command line interface, Arcanist.

:doc:`HowToSubmitABug`
Instructions for properly submitting information about any bugs you run into
in the LLVM system.
Expand All @@ -54,7 +49,7 @@ LLVM welcomes contributions of all kinds. To get started, please review the foll
efficient C++ code.

:doc:`GitHub`
Describes how to use the llvm-project repository on GitHub.
Describes how to use the llvm-project repository and code reviews on GitHub.

:doc:`GitBisecting`
Describes how to use ``git bisect`` on LLVM's repository.
Expand Down Expand Up @@ -110,7 +105,7 @@ Discourse forums. There are also commit mailing lists for all commits to the LLV
The :doc:`CodeOfConduct` applies to all these forums and mailing lists.

`LLVM Discourse`__
The forums for all things LLVM and related sub-projects. There are categories and subcategories for a wide variety of areas within LLVM. You can also view tags or search for a specific topic.
The forums for all things LLVM and related sub-projects. There are categories and subcategories for a wide variety of areas within LLVM. You can also view tags or search for a specific topic.

.. __: https://discourse.llvm.org/

Expand All @@ -130,9 +125,9 @@ The :doc:`CodeOfConduct` applies to all these forums and mailing lists.
.. __: http://lists.llvm.org/pipermail/llvm-bugs/

`LLVM Announcements`__
If you just want project wide announcements such as releases, developers meetings, or blog posts, then you should check out the Announcement category on LLVM Discourse.
If you just want project wide announcements such as releases, developers meetings, or blog posts, then you should check out the Announcement category on LLVM Discourse.

.. __: https://discourse.llvm.org/c/announce/46
.. __: https://discourse.llvm.org/c/announce/46

.. _online-sync-ups:

Expand All @@ -146,7 +141,7 @@ The :doc:`CodeOfConduct` applies to all online sync-ups.

If you'd like to organize a new sync-up, please add the info in the table
below. Please also create a calendar event for it and invite [email protected]
to the event, so that it'll show up on the :ref:`llvm-community-calendar`.
to the event, so that it'll show up on the :ref:`llvm-community-calendar`.
Please see :ref:`llvm-community-calendar-host-guidance` for more guidance on
what to add to your calendar invite.

Expand Down Expand Up @@ -291,7 +286,7 @@ don't find anyone present, chances are they happen to be off that day.
- `Video Call <https://meet.grosser.science/LLVMOfficeHours>`__
- English, German, Spanish, French
* - Anastasia Stulova
- Clang internals for C/C++ language extensions and dialects, OpenCL, GPU, SPIR-V, how to contribute, women in compilers.
- Clang internals for C/C++ language extensions and dialects, OpenCL, GPU, SPIR-V, how to contribute, women in compilers.
- Monthly, 1st Tuesday of the month at 17:00 BST - London time (9:00am PT except for 2 weeks in spring), 30 mins slot.
- `GoogleMeet <https://meet.google.com/kdy-fdbv-nuk>`__
- English, Russian, German (not fluently)
Expand Down Expand Up @@ -330,7 +325,7 @@ Guidance for office hours hosts

* If you're interested in becoming an office hours host, please add your
information to the list above. Please create a calendar event for it and
invite [email protected] to the event so that it'll show up on the
invite [email protected] to the event so that it'll show up on the
:ref:`llvm-community-calendar`.
Please see :ref:`llvm-community-calendar-host-guidance` for more guidance on
what to add to your calendar invite.
Expand Down Expand Up @@ -446,7 +441,7 @@ Guidance on what to put into LLVM community calendar invites

To add your event, create a calendar event for it and invite [email protected]
on it. Your event should then show up on the community calendar.

Please put the following pieces of information in your calendar invite:

* Write a single paragraph describing what the event is about. Include things
Expand Down
Loading