Arcanist deprecation errors on PHP 8

Project Infrastructure
1

Arcanist continues to not age well.

For anyone who is experiencing an error like:

$ arc patch D127907                                                                                                                                                             
 Exception                                                                                                                                                                                                         
preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated                                                                                                                                 
(Run with `--trace` for a full exception trace.)

This is caused by a deprecation warning being treated as an error. I really hate that I have to know anything about PHP to handle code reviews in LLVM, but apparently, this starts happening after an upgrade to PHP 8. I am already using arcanist as cloned from git, so I just applied the following patch to tell it to use the default error flags (which at least on my machine, disable deprecation warnings in php.ini).

diff --git a/support/init/init-script.php b/support/init/init-script.php
index ad40b8bd..5da2e045 100644
--- a/support/init/init-script.php
+++ b/support/init/init-script.php
@@ -15,7 +15,7 @@ function __arcanist_init_script__() {
     ob_end_clean();
   }
 
-  error_reporting(E_ALL | E_STRICT);
+  //error_reporting(E_ALL | E_STRICT);
 
   $config_map = array(
     // Always display script errors. Without this, they may not appear, which is
8 Likes
2

This is helpful. Thanks !

3

Just FYI, for arc diff the following patch may be needed for some newer PHP versions

diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php
index 7201a003..808dc9bd 100644
--- a/src/workflow/ArcanistDiffWorkflow.php
+++ b/src/workflow/ArcanistDiffWorkflow.php
@@ -2360,6 +2360,9 @@ EOTEXT

     // If we track an upstream branch either directly or indirectly, use that.
     $branch = $api->getBranchName();
+    if (is_null($branch)) {
+        $branch = '';
+    }
     if (strlen($branch)) {
       $upstream_path = $api->getPathToUpstream($branch);
       $remote_branch = $upstream_path->getRemoteBranchName();

It’s also quite straight-forward to use the Phabricator API for the main tasks. For example, I’ve been using the following script as replacement for arc patch: ⚙ D73075 [utils] Add initial llvm-patch helper to manage patches. (with the caveat that it only supports patches that apply cleanly)

2 Likes
4

I’ve had some success using moz-phab to interact with Phabricator instead of arcanist. I haven’t switched to it as my daily driver yet out of sheer laziness, but it worked pretty well for everything I tried.

3 Likes
5

I hit the same error when I try to apply patch ⚙ D130205 [Darwin toolchain] Tune the logic for finding arclite.

Didn’t dig deeper trying to figure out the root cause but the following change allow me to succeed:

diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index 6c6d2ac4..dc0b63f6 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -1142,6 +1142,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
   }

   public function hasLocalCommit($commit) {
+    if (!$commit) {
+      return false;
+    }
     try {
       if (!$this->getCanonicalRevisionName($commit)) {
         return false;
1 Like
6

If we get some +1’s on daily use of moz-phab, I think we should change our docs to recommend it. Arcanist is aging badly and i daresay it is none of our ambitions to know much about php in order to submit code to llvm.

1 Like
7

I use moz-phab daily, works great. Only thing that I really add on top is forced addition of the --no-bug --no-wip options.

– River

1 Like
8

I never used moz-phab but I would happy to try and live on it if someone write up a doc about how to use that with llvm phabricator.

9

I’m in the same boat – haven’t used it but am tired of trying to keep arcanist patched across my setups. @River707 do you recall if it was harder than just RTFM and install/go?

10

I guess we are moving to GitHub review: Code Review Process Update

So it might not be worth documenting moz-phab just for the few months before transition.

11

If we’re moving to GitHub reviews, then yeah, nothing needed here. I had somewhat stopped paying attention to the status of that and assumed that someone would announce the plan and date – which looks like it has been done. Still 1y+ out. That still leaves a lot of room for arcanist pain for developers. If moz-phab is working, I think it would help folks if we wrote that down as an alternative to arcanist at the least.

12

Just to comment, if arcanist doesn’t work, the plain upload a patch through the web interface one does. For most folks, I suspect arcanist is overkill.

13

I might just be thick, but I did do this the last time arcanist gave me problems – and I thoroughly messed everything up. Somewhat embarrassing to admit, but I may have grown dependent on the overkill.

14

From my recollection, the only thing I did was install (by reading the docs) and create a little wrapper script:

#!/bin/bash

moz-phab --no-bug --no-wip --upstream upstream/main "$@"

(upstream is the name of the LLVM remote in my fork)

I don’t really like using the manual patch upload. I never remember how to create the patch correctly, and it always feels clunky.

15

A manually uploaded patch is probably what caused the tool to fail in the first place in my case. arc patch will not only apply the patch but also include the reviewer and review link which is kind of useful when I need to commit other people’s patch.

Telling everyone to manually upload the patch might just mean to kill arc, at least don’t run it with php 8.

16

Doesn’t arcanist upload some additional metadata as well (i.e. the base commit)? Agreed - the flow to be able to apply someone else’s patch (or my own on another machine) is something I rely on a lot and I fumble it a lot when it doesn’t work.

17

I committed a patch for arcanist in: ⚙ D129232 [llvm][docs] commit phabricator patch. We might need to carry more…

18

Like: ⚙ D131699 add arcanist patch

19

Maybe we should fork arcanist and apply all patches there instead? We are still going to stick with Phab for a year - so making it easier for new people to get arc is important.

1 Like
20
1 Like