Merge pull request #313 from jhwgh1968/warn_compiler_targets

Warn on updates to compiler targets

Author: pietroalbini

highfive/newpr.py

@@ -29,13 +29,15 @@
 
 warning_summary = ':warning: **Warning** :warning:\n\n%s'
 submodule_warning_msg = 'These commits modify **submodules**.'
+targets_warning_msg = 'These commits modify **compiler targets**. (See the [Target Tier Policy](https://doc.rust-lang.org/nightly/rustc/target-tier-policy.html).)'
 surprise_branch_warning = "Pull requests are usually filed against the %s branch for this repo, but this one is against %s. Please double check that you specified the right target!"
 
 review_with_reviewer = 'r? @%s\n\n(rust-highfive has picked a reviewer for you, use r? to override)'
 review_without_reviewer = '@%s: no appropriate reviewer found, use r? to override'
 
 reviewer_re = re.compile("\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)")
 submodule_re = re.compile(".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE)
+target_re = re.compile("^[+-]{3} [ab]/compiler/rustc_target/src/spec/", re.MULTILINE)
 
 rustaceans_api_url = "http://www.ncameron.org/rustaceans/user?username={username}"
 
@@ -85,6 +87,9 @@ def _load_json_file(self, name):
     def modifies_submodule(self, diff):
         return submodule_re.match(diff)
 
+    def modifies_targets(self, diff):
+        return target_re.search(diff)
+
     def api_req(self, method, url, data=None, media_type=None):
         data = None if not data else json.dumps(data).encode("utf-8")
         headers = {} if not data else {'Content-Type': 'application/json'}
@@ -164,6 +169,9 @@ def post_warnings(self, diff, owner, repo, issue):
         if self.modifies_submodule(diff):
             warnings.append(submodule_warning_msg)
 
+        if self.modifies_targets(diff):
+            warnings.append(targets_warning_msg)
+
         if warnings:
             self.post_comment(warning_summary % '\n'.join(map(lambda x: '* ' + x, warnings)), owner, repo, issue)
 

highfive/tests/fakes/target.diff

@@ -0,0 +1,12 @@
+diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs
+index 039e9a8b274..43a8c5e415d 100644
+--- a/compiler/rustc_target/src/spec/mod.rs
++++ b/compiler/rustc_target/src/spec/mod.rs
+@@ -680,6 +680,7 @@ fn to_json(&self) -> Json {
+     ("aarch64-linux-android", aarch64_linux_android),
+ 
+     ("x86_64-linux-kernel", x86_64_linux_kernel),
++    ("x86_64-freebsd-kernel", x86_64_freebsd_kernel),
+ 
+     ("aarch64-unknown-freebsd", aarch64_unknown_freebsd),
+     ("armv6-unknown-freebsd", armv6_unknown_freebsd),

highfive/tests/test_newpr.py

@@ -284,6 +284,20 @@ def test_submodule(self):
         normal_diff = load_fake('normal.diff')
         assert not handler.modifies_submodule(normal_diff)
 
+        targets_diff = load_fake('target.diff')
+        assert not handler.modifies_submodule(targets_diff)
+
+    def test_targets(self):
+        handler = HighfiveHandlerMock(Payload({})).handler
+        targets_diff = load_fake('target.diff')
+        assert handler.modifies_targets(targets_diff)
+
+        normal_diff = load_fake('normal.diff')
+        assert not handler.modifies_targets(normal_diff)
+
+        submodule_diff = load_fake('submodule.diff')
+        assert not handler.modifies_targets(submodule_diff)
+
     def test_expected_branch_default_expected_no_match(self):
         payload = Payload(
             {'pull_request': {'base': {'label': 'repo-owner:dev'}}}
@@ -631,6 +645,7 @@ def tpw_defaults(cls, patcherize):
         cls.mocks = patcherize((
             ('unexpected_branch', 'highfive.newpr.HighfiveHandler.unexpected_branch'),
             ('modifies_submodule', 'highfive.newpr.HighfiveHandler.modifies_submodule'),
+            ('modifies_targets', 'highfive.newpr.HighfiveHandler.modifies_targets'),
             ('post_comment', 'highfive.newpr.HighfiveHandler.post_comment'),
         ))
 
@@ -654,23 +669,27 @@ def post_warnings(self):
     def test_no_warnings(self):
         self.mocks['unexpected_branch'].return_value = False
         self.mocks['modifies_submodule'].return_value = False
+        self.mocks['modifies_targets'].return_value = False
 
         self.post_warnings()
 
         self.mocks['unexpected_branch'].assert_called_once_with()
         self.mocks['modifies_submodule'].assert_called_with(self.diff)
+        self.mocks['modifies_targets'].assert_called_with(self.diff)
         self.mocks['post_comment'].assert_not_called()
 
     def test_unexpected_branch(self):
         self.mocks['unexpected_branch'].return_value = (
             'master', 'something-else'
         )
         self.mocks['modifies_submodule'].return_value = False
+        self.mocks['modifies_targets'].return_value = False
 
         self.post_warnings()
 
         self.mocks['unexpected_branch'].assert_called_once_with()
         self.mocks['modifies_submodule'].assert_called_with(self.diff)
+        self.mocks['modifies_targets'].assert_called_with(self.diff)
 
         expected_warning = """:warning: **Warning** :warning:
 
@@ -682,11 +701,13 @@ def test_unexpected_branch(self):
     def test_modifies_submodule(self):
         self.mocks['unexpected_branch'].return_value = False
         self.mocks['modifies_submodule'].return_value = True
+        self.mocks['modifies_targets'].return_value = False
 
         self.post_warnings()
 
         self.mocks['unexpected_branch'].assert_called_once_with()
         self.mocks['modifies_submodule'].assert_called_with(self.diff)
+        self.mocks['modifies_targets'].assert_called_with(self.diff)
 
         expected_warning = """:warning: **Warning** :warning:
 
@@ -700,11 +721,13 @@ def test_unexpected_branch_modifies_submodule(self):
             'master', 'something-else'
         )
         self.mocks['modifies_submodule'].return_value = True
+        self.mocks['modifies_targets'].return_value = False
 
         self.post_warnings()
 
         self.mocks['unexpected_branch'].assert_called_once_with()
         self.mocks['modifies_submodule'].assert_called_with(self.diff)
+        self.mocks['modifies_targets'].assert_called_with(self.diff)
 
         expected_warning = """:warning: **Warning** :warning:
 
@@ -714,6 +737,28 @@ def test_unexpected_branch_modifies_submodule(self):
             expected_warning, self.owner, self.repo, self.issue
         )
 
+    def test_unexpected_branch_modifies_submodule_and_targets(self):
+        self.mocks['unexpected_branch'].return_value = (
+            'master', 'something-else'
+        )
+        self.mocks['modifies_submodule'].return_value = True
+        self.mocks['modifies_targets'].return_value = True
+
+        self.post_warnings()
+
+        self.mocks['unexpected_branch'].assert_called_once_with()
+        self.mocks['modifies_submodule'].assert_called_with(self.diff)
+        self.mocks['modifies_targets'].assert_called_with(self.diff)
+
+        expected_warning = """:warning: **Warning** :warning:
+
+* Pull requests are usually filed against the master branch for this repo, but this one is against something-else. Please double check that you specified the right target!
+* These commits modify **submodules**.
+* These commits modify **compiler targets**. (See the [Target Tier Policy](https://doc.rust-lang.org/nightly/rustc/target-tier-policy.html).)"""
+        self.mocks['post_comment'].assert_called_with(
+            expected_warning, self.owner, self.repo, self.issue
+        )
+
 
 class TestNewPrFunction(TestNewPR):
     @pytest.fixture(autouse=True)