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 git@github.com:<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 git@rose-github.llnl.gov:<user_oun>/rose.git
   16101fd..563b510  central/master -> master

master 無法同步

在極少數情況下，您的儲存庫的 master 分支無法自動同步。這很可能是由於合併衝突造成的。您將透過自動電子郵件收到錯誤訊息，類似於以下內容（最後更新於 2012 年 7 月 24 日）

To git@rose-github.llnl.gov:lin32/rose.git
! [rejected]        origin/master -> master (non-fast forward)
error: failed to push some refs to 'git@rose-github.llnl.gov: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

參考文獻

動機

目標

軟體