ROSE 编译器框架/代码审查

请注意：内部 github 的 URL 已更改！现在是 https://rose-github.llnl.gov/，而不是 https://github.llnl.gov/ 。

动机

没有代码审查，开发人员会

添加了不可读的贡献，这些贡献不符合任何一致的编码风格。
添加了未记录的贡献，其他人无法理解（本质上是无用的贡献）。
添加了未测试的贡献（没有附带测试的代码），因此贡献无法按预期工作，或者很容易被其他冲突的贡献破坏（另一个本质上不太有用的贡献）
禁用测试来破坏我们严格的 Jenkins CI 回归测试
将文件添加到错误的目录中，使用不正确的名称
提交了数百个重新格式化的文件
重新发明轮子，实现了已经存在的特性
将 160MB 的 MPI 跟踪文件添加到 git 存储库中

请参阅 Phabricator 的“审查优势”文档（Facebook 项目）。

目标

我们对 ROSE 代码审查的主要目标是

分享关于代码的知识：编码者 + 审查者将了解代码，而不是仅仅是编码者
小组学习：通过学习其他人的代码来学习
执行一致的 ROSE 代码可用性和可维护性的策略：已记录和经过测试
避免重复造轮子，消除不必要的冗余
保护代码：不允许破坏性地尝试禁用或删除回归测试

软件

我们目前正在测试 Github 企业版，并正在研究利用 Redmine 进行内部代码审查的可能性。

过去，我们已经研究过 Google 的 Gerrit 代码审查系统。

Github

版本：https://enterprise.github.com/releases

支持：https://support.enterprise.github.com

rosebot

(开发中)

一个自动化的拉取请求分析器，用于执行各种任务

根据分层配置自动将审查者添加到拉取请求中
“预接收挂钩”分析：文件大小、文件数量、专有源代码等。
更多...

开发者清单

在发送代码审查请求之前，请阅读这些提示和指南。

编码规范

请访问编码规范以获取完整的指南。这里我们只总结一些要点。

您的代码应该以易于维护和审查的方式编写

编写易于理解的代码；避免使用任何人都难以理解的奇特技术。
添加足够的文档（源代码注释、README 等）以帮助理解您的代码，您的文档应涵盖
- 为什么要这样做（动机）
- 如何做到这一点（设计和/或算法）
- 关联的测试在哪里（按预期工作）
在提交您的代码以供审查之前，请确保
- 您已合并到最新的中心存储库的 master 分支中，没有任何冲突
- 您的工作副本可以通过以下方式通过本地测试：make、make check 和 make distcheck
- 您已尽可能修复了代码的所有编译器警告
提交一个逻辑工作单元（一个或多个提交）；一些连贯的东西，比如修复 bug、改进文档、达到大型新功能的中间阶段。
以良好的 [代码行数] 和 [代码复杂度] 比例平衡代码提交。为了让审查者的工作更轻松，需要取得良好的平衡。
- 审查您的代码所需的时间不应超过 1 小时。请避免一次推送数千行代码。
- 也请避免推送任何琐碎的（修复了错字、注释掉了单行等）以供审查。

一次性设置

初始化代码审查的步骤

1. 使用您的 OUN 和 PAC 登录到 http://rose-github.llnl.gov。

2. 从 http://rose-github.llnl.gov/rose-compiler/rose 分叉您自己的 ROSE 存储库克隆。

访问 http://rose-github.llnl.gov/rose-compiler/rose
单击网页右上角的分叉按钮

3. 添加协作者

访问 http://rose-github.llnl.gov/<您的帐户>/rose
- 单击管理
- 单击协作者
  - 添加候选代码审查者：liao6、too1。这些开发人员将审查并合并您的工作。
  - 添加管理员：hudson-rose。此用户将自动将您的 master 分支与 /nfs/casc/overture/ROSE/git/ROSE.git:master 同步。

4. 使用以下命令创建您的公钥-私钥 SSH 密钥对ssh-keygen，并将公钥添加到您的 rose-github.llnl.gov 帐户。请参考生成 SSH 密钥或者使用您已有的公钥。（rose-github.llnl.gov 目前仅支持 SSH 协议；HTTPS 尚未支持。）

5. 配置自动同步：联系 Jenkins 管理员（too1 和 liao6）将您的仓库添加到白名单，以便在新的提交集成到 ROSE 的官方主分支时进行同步。

6. 设置轮询作业：联系 Jenkins 管理员（too1 和 liao6）让您的 Github 仓库轮询主分支上的新变更。当检测到新变更时，您的主分支将被推送到中央仓库（并添加到 Jenkins 测试队列）为<oun>-reviewd-rc。

日常工作流程

使用本地 git 仓库进行工作并提交本地提交，您有两个选择
- 从 /nfs/casc/overture/rose/rose.git 克隆，就像我们之前通常做的那样
- 将您在 rose-github.llnl.gov 上的分叉克隆到本地仓库（仅通过 LC 支持 HTTPS）

注意：您可能会遇到 SSL 证书问题。如果出现问题，只需使用 export GIT_SSL_NO_VERIFY=false 或配置 git 来禁用 cURL 中的 SSL 验证

$ git config --global http.sslVerify false

- 不要使用分支，为每个任务使用独立的 git 仓库。因此，一项任务的状态/进度不会影响其他任务。
准备好推送您的提交时，请与最新的 rose-compiler/master 同步以解决合并冲突等。
- 输入：git pull origin master # 这应该始终有效，因为 rose-github.llnl.gov 上的主分支会自动保持最新状态
- 确保您的本地更改能通过 1)make -j8，2)make check -j8 和 3)make distcheck -j8
将您的提交推送到您分叉的非主分支（如 bugfix-rc，featurex-rc，work-status 等）。您可以在您的分叉仓库中创建任何分支，并使用您喜欢的任何名称

  # If your local repository was cloned from /nfs/casc/overture/ROSE/rose.git. 
  # There is no need to discard it. You can just add the rose-github.llnl's repo as an additional remote repository and push things there:
  git remote add github-llnl-youraccount-rose http://rose-github.llnl.gov/youraccount/rose.git
  git push github-llnl-youraccount-rose HEAD:refs/heads/bugfix-rc

- 建议将您的工作推送到带有 -status 后缀的远程分支，这将触发一个预筛选 Jenkins 作业：http://hudson-rose-30:8080/view/Status/job/S0-pre-screening-before-code-review/。这在确保您的推送能通过最小化 make check 规则（包括您自己的规则）之前，审阅者花时间阅读您的代码之前，通常很有用。审阅者还可以看到您的代码和代码的操作。
添加一个 pull（合并）请求将 bugfix-rc 合并到您自己的分叉主分支中，
- 请注意，默认的 pull 请求将使用 rose-compiler/rose 的主分支作为基础分支（合并的目标）。请改为将其更改为您自己的分叉的主分支。
- 还要确保 pull（合并）请求的源（头部）分支是您想要的（在本例中为 bugfix-rc）
- 仔细检查 pull 请求的 diff 选项卡，仅显示您所做的更改，不包括从中央仓库引入的其他内容。或者您自己的仓库主分支与中央仓库主分支不同步。通知系统管理员（too1）出现此问题，或使用本页面的故障排除部分手动修复。
通知审阅者您有一个 pull 请求（请求将您的 bugfix-rc 合并到您的主分支中）
- 您可以将 pull 请求分配给审阅者，以便自动向审阅者发送电子邮件通知。
- 或者您可以在 pull 请求中使用 @revieweraccount 添加讨论。注意：请只点击“评论此问题”一次，然后手动刷新网页。Github Enterprise 有一个错误，因此无法自动显示新添加的评论。 bug79
- 或者您也可以直接给审阅者发送电子邮件
等待审阅者的反馈

审查结果

完成并提交到 Jenkins
- 如果您的代码通过代码审查，审阅者应该已经将您的 bugfix-rc 合并到您的主分支中。Jenkins 将自动轮询您的主分支并进行测试/合并
如何进行更改
- 要实施更改，请进行本地编辑、本地提交、推送到您的远程分支，然后再次发送合并请求
认真对待代码审查
- 请记住，代码审查不是对您个人进行攻击。代码审查的目的是让同事评估您的代码。这可能需要合理的时间，因此请尊重他们的努力，并认真重新审视您的代码。
- 查看审阅者的评论并解决它们，或者评论代码的用途，然后等待回复
- 有些评论是强制性更改，这些更改必须在您通过代码审查之前解决
- 有些评论是建议。您应该仔细考虑他们的建议。如果审阅者建议您做一些事情，您应该为差异制定合理化方案，或者考虑更改代码的影响。ROSE 是团队合作的结果，我们必须认真对待同事。
- 不要关闭pull 请求。您可以再次将新的提交推送到同一个分支，并在 pull 请求中添加评论以表明有新的更新。请再次审查它们。这将避免不必要的重复。

代码审查的好处

避免编写已经存在的特性。
- 请记住，您是在为用户编写代码，我们必须尽力编写清晰的代码
- 代码一致性在大型项目中极其重要。一致的代码允许用户将时间花在自己的项目上，而不是试图在 doxygen 页面中找到答案，并发现七、八种完成相同任务的方法，却不知道不同方法的后果
团队编码
- 如果每个程序员都隐藏起来，不顾 ROSE 已经拥有的功能，独自编码，不仅会让用户感到困惑，还会让开发人员感到困惑。
- 乍一看，ROSE 源代码目录的体积接近 1 GB。在进行 make、make check、make install、make installcheck、make distcheck 之后，编译目录的体积为 19G。教训：ROSE 很大，一个人了解 ROSE 及其功能的所有内容的机会很渺茫
- 作为一个团队，规模相当大，但只要每个人都在自己特定的部分上工作，并询问可能存在的特性重复和代码可读性，就可以管理。

审阅者清单

作为代码审阅者要注意什么？

熟悉当前的编码规范作为执行代码审查的一般指南。
一次最多分配 1 小时的时间来审查大约 500-1000 行代码：时间更长可能不会带来回报，因为人类大脑的注意力跨度有限

检查内容

要检查的六个主要内容

文档：提交内容是什么？这是否反映在 README、源代码注释或 LaTex 文件中？
样式：编码样式是否符合我们的标准？代码是否干净、健壮且易于维护？
接口：代码是否具有用户易于使用的简洁明了的接口？
算法：代码中是否有关于使用何种算法的足够注释？算法是否正确且高效（空间和时间复杂度）？
实现：代码是否正确地实现了已记录的算法？
测试：代码是否有配套的测试翻译器和输入测试代码以确保贡献能按预期执行？
- Jenkins 是否已配置为触发这些测试（您的工作可能需要新的先决条件软件或配置选项）？开发者工作站上的本地测试不算数。

更多详细信息，来自编码规范的快速摘要

命名约定：文件和目录名称遵循我们的标准；清晰直观
- 目录结构：源代码、测试代码和文档文件已添加到正确的位置
可维护性：代码清晰度；没有编写代码的人能否轻松理解代码的功能？
- 没有过长的函数：一个函数包含数百行代码是不可取的
- 架构/设计：编写代码的原因和动机，以及其设计。
没有重复：可能已经存在类似代码，或者可以扩展现有代码
重复使用：代码的一部分是否可以重构为可供其他人重复使用？
单元测试：make check 规则与每个新功能相关联，以确保新功能将被测试和验证以确保预期行为
健全性：不要关闭或放松其他测试，以使开发者的提交通过 Jenkins。换句话说，不要作弊。

决定

对代码审查做出明确而最终的决定

通过：代码按预期运行，并有清晰的文档和测试用例。 合并并关闭拉取请求。
通过但有未来任务。提交已接受。但将来需要一些额外的任务来改进代码。它们可以放入一组单独的提交中，并在稍后推送。
失败。需要额外的工作，例如更好的命名，更好的文件放置位置，更多源代码注释，添加回归测试等。通知开发者问题并要求推送新的提交集以解决更正或改进。

给出负面反馈

我们直接引用自 http://www.mediawiki.org/wiki/Code_review_guide#Giving_negative_feedback

" 以下是在您需要拒绝某人的提交或要求他们清理工作时的一些指南

将您的评论集中在代码和任何客观观察到的行为上，而不是动机；例如，不要陈述或暗示关于动机因素的假设，例如开发者是否只是太懒或太笨而无法正确做事。
要有同理心和善意。认识到开发者可能在他们的想法上付出了很多努力，如果您觉得这样做很舒服和真诚，就感谢他们的贡献（并尽量鼓起勇气和真诚）。最重要的是，设身处地地为他们着想，并说一些表明你已经这样做了的话。
帮助他们安排工作。如果他们的想法是“还没有”的那种想法，试着推荐你所知道的将他们的想法放到积压工作中的最佳方法（即最有可能最终被重新审视的积压工作）。
让他们知道他们可以在哪里对您的决定提出上诉。例如，如果贡献者没有被认为是破坏性或愚蠢的历史，请邀请他们在 wikitech-l 上讨论这个问题。
要明确。不要说得太含糊，以至于掩盖了中心信息。
最重要的是，要尽快给出反馈。虽然委婉地说更好（你应该从过去的错误中吸取教训），但你总是可以为评论表达不当而道歉，并迅速跟进。不要把负面反馈留给别人，也不要希望他们不够坚持，以至于无法让他们的贡献生效。"

谁应该审查什么

理想情况下，每个 ROSE 贡献者都应该在某个时间点作为审阅者参与代码审查，这样才能充分发挥同行评审的优势。

但是，由于对我们内部 github 企业服务器的访问权限有限，我们目前有一个集中式审查流程，其中 ROSE 员工（liao6，too1）担任默认代码审阅者。他们负责自己审查代码或委托给其他开发者，这些开发者要么对贡献有更多了解，要么应该了解贡献。

我们正在积极寻找更好的选择，并将逐渐扩大审阅者队伍，这样审查步骤就不会成为瓶颈。

待办事项：使用rosebot根据源代码树的分层配置自动分配审阅者。

应该避免什么

根据审阅者会写什么来判断代码
- 给定一个问题，通常有十多种不同的方法可以解决它。并且给定一个解决方案，有一百万种方法可以将其渲染成代码。
退化为吹毛求疵
- 完美主义可能会损害进度。我们应该允许一些非关键的改进在下一个版本/提交中完成。
觉得有义务说一些批评性的东西：说“看起来不错，通过”是完全可以的
审查延迟：我们不应该急于求成，但我们应该记住，有人在等待审查完成才能前进

批评

代码审查经常退化为吹毛求疵。头脑风暴和设计审查更有效率。

这是有道理的，越早发现问题，越好。设计发生得更早。应该审查设计。同样的想法也适用于需求分析。
为了降低这种风险，我们现在在我们的编码标准中制定了设计文档规则。

故障排除

master 不同步

每个开发者 git 存储库的 master 分支 (http://rose-github.llnl.gov) 应该自动与中央 git 存储库的 master 分支同步 (/nfs/casc/overture/ROSE/git/ROSE.git)。在极少数情况下，它可能不同步。以下是如何执行手动同步的示例

1. 克隆您的 Github 存储库

$ cd ~/Development/projects/rose
$ git clone [email protected]:<user_oun>/rose.git
Cloning into ROSE...
remote: Counting objects: 216579, done.
remote: Compressing objects: 100% (55675/55675), done.
remote: Total 216579 (delta 159850), reused 211131 (delta 155786)
Receiving objects: 100% (216579/216579), 296.41 MiB | 35.65 MiB/s, done.
Resolving deltas: 100% (159850/159850), done.

2. 将中央存储库添加为远程存储库

$ git remote add central /nfs/casc/overture/ROSE/git/ROSE.git
$ git fetch central
From /nfs/casc/overture/ROSE/git/ROSE.git
 * [new branch]      master     -> central/master
 ...

3. 将中央 master 分支推送到您的 Github 的 master 分支

-bash-3.2$ git push central central/master:refs/heads/master
Total 0 (delta 0), reused 0 (delta 0)
To [email protected]:<user_oun>/rose.git
   16101fd..563b510  central/master -> master

master 无法同步

在极少数情况下，您的存储库的 master 分支无法自动同步。这很可能是由于合并冲突造成的。您将通过自动电子邮件收到错误消息，类似于以下内容（最后更新于 2012 年 7 月 24 日）

To [email protected]:lin32/rose.git
! [rejected]        origin/master -> master (non-fast forward)
error: failed to push some refs to '[email protected]:lin32/rose.git'

---

Your master branch at [rose-github.llnl.gov:lin32/rose.git] cannot be automatically updated with [/nfs/casc/overture/ROSE/git/ROSE.git:master]

Please manually force the update:

Add the central repository as a remote, call it "nfs":

  $ git remote add nfs /nfs/casc/overture/ROSE/git/ROSE.git

1. First, try to manually perform a merge in your local repository:

  # 1. Checkout and update your Github's master branch
  $ git checkout master
  $ git pull origin master

  # 2. Merge the central master into your local master
  $ git pull nfs master
  <no merge conflicts>

  # 3. Synchronize your local master to your Github's master
  $ git push origin HEAD:refs/head/master

2. Otherwise, try to resolve the conflict.

3. Finally, if all else fails, force the synchronization:

  $ git push --force origin nfs/master:refs/heads/master

  WARNING: your master branch on Github will be overriden so make sure
  you have sufficient backups, and take precaution.

请简单地按照电子邮件中的说明强制更新您的 Github 的 master 分支。

过去软件经验

过去，我们尝试过其他代码审查工具

Gerrit (Google)

简而言之

Gerrit 的用户界面不友好（它很复杂，因此更令人困惑）。这是真的，与 Github 的拉取请求机制相比，用于代码审查。
Gerrit 的远程 API 还不够成熟，无法处理我们的工作流程。此外，我们不得不进行一些修改，以稍微满足我们的需求。另一方面，Github 有一个很棒的远程 API，可以通过 Ruby 脚本轻松访问，Ruby 是一种非常流行的用于 Web 界面和开发领域的语言。
Gerrit 不像 Github 那样流行，这对我们的项目获得关注很重要。此外，更多人熟悉 Github，因此更容易使用。

待办事项

最高优先级：在手动代码审查启动之前添加预筛选 Jenkins 作业
研究、安装和测试 Facebook 的 Phabricator： http://phabricator.org/

与 Jenkins 的连接

参见 Continuous_Integration#Connection_to_Code_Review

参考文献

动机

目标

软件