This is an archive of the discontinued LLVM Phabricator instance.

Differential D131699

add arcanist patch
ClosedPublic

Authored by justinstitt on Aug 11 2022, 10:42 AM.

Details

Reviewers
nickdesaulniers
hiraditya
foad
Commits
rG11a2b44ad412: add arcanist patch to fix arc diff
Summary

Another patch to fix arcanist on newer php versions. See previous patch
here: https://reviews.llvm.org/D129232

Diff Detail

Event Timeline

justinstitt created this revision.Aug 11 2022, 10:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 10:42 AM
justinstitt requested review of this revision.Aug 11 2022, 10:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 10:42 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B180718: Diff 451912.Aug 11 2022, 12:58 PM
nickdesaulniers added reviewers: nickdesaulniers, hiraditya.Aug 22 2022, 9:55 AM
nickdesaulniers added a comment.Aug 22 2022, 10:06 AM

The patch LGTM; I'd like @hiraditya to test it though first to see if it resolves https://github.com/llvm/llvm-project/issues/57271.

hiraditya added a comment.Aug 23 2022, 6:34 AM

Not working either. Do I have to reboot the machine/restart a service ?
Machine type:

lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 22.04.1 LTS
Release:	22.04
Codename:	jammy
hiraditya added a comment.Aug 23 2022, 6:35 AM

with arc diff

Deprecated: Function libxml_disable_entity_loader() is deprecated in /usr/share/arcanist/support/init/init-script.php on line 92
PHP Fatal error:  Uncaught Exception: Error while loading file "/usr/share/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /usr/share/arcanist/src/init/lib/PhutilBootloader.php:275
Stack trace:
#0 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#1 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#2 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#3 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 /usr/share/arcanist/src/filesystem/Filesystem.php(18): __phutil_autoload()
#5 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(247): include_once('...')
#6 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#7 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#8 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#9 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#10 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(97): __phutil_autoload()
#11 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(21): PhutilBootloader->registerLibrary()
#12 /usr/share/arcanist/src/init/init-library.php(70): PhutilBootloader::newLibrary()
#13 /usr/share/arcanist/support/init/init-script.php(96): require_once('...')
#14 /usr/share/arcanist/support/init/init-script.php(115): __arcanist_init_script__()
#15 /usr/share/arcanist/support/init/init-arcanist.php(3): require_once('...')
#16 /usr/share/arcanist/bin/arc(10): require_once('...')
#17 {main}
  thrown in /usr/share/arcanist/src/init/lib/PhutilBootloader.php on line 275
hiraditya accepted this revision.Aug 26 2022, 5:22 PM
hiraditya mentioned this in D132523: Fixes to make arcanist compatible with php8.1 on Linux.

LGTM

This revision is now accepted and ready to land.Aug 26 2022, 5:23 PM
hiraditya added a comment.Aug 26 2022, 5:24 PM

More changes were needed to make it work on my machine. I've added them in a separate diff: https://reviews.llvm.org/D132523

hiraditya added a comment.EditedSep 3 2022, 7:32 AM

ping: do we plan to merge this patch?

nickdesaulniers requested changes to this revision.Sep 29 2022, 10:41 AM

I think we probably need more fixes than this at this point. Thanks for the patch though!

This revision now requires changes to proceed.Sep 29 2022, 10:41 AM
tomhughes added a subscriber: tomhughes.Oct 25 2022, 11:47 AM

FWIW, I needed both https://github.com/llvm/llvm-project/blob/main/llvm/utils/phabricator/0001-Fix-PHP-8.1-incompatibility-with-arc-patch-D-12345.patch and this patch.

foad accepted this revision.Jan 20 2023, 7:13 AM
foad added a subscriber: foad.
In D131699#3824505, @nickdesaulniers wrote:

I think we probably need more fixes than this at this point. Thanks for the patch though!

This works for me, and fixes an annoying problem when I run arc diff when I'm not on any local git branch. Why would you reject one fix just because unspecified "more fixes" are also needed?

Herald added a subscriber: StephenFan. · View Herald TranscriptJan 20 2023, 7:13 AM
dblaikie added a subscriber: dblaikie.Jan 23 2023, 8:34 AM
hiraditya added a comment.Jan 23 2023, 5:22 PM

if this patch helps make progress for some folks, i think it is reasonable to merge this one as well. @nickdesaulniers do you have any objections to merging this patch?

nickdesaulniers added subscribers: yihanaa, bhuvanendrakumarn, Cooper-Qu and 6 others.Jan 23 2023, 5:24 PM

You guys can merge. Justin doesn't have commit access.

This revision was not accepted when it landed; it landed in state Needs Revision.Feb 1 2023, 12:14 PM
Closed by commit rG11a2b44ad412: add arcanist patch to fix arc diff (authored by hiraditya). · Explain Why
This revision was automatically updated to reflect the committed changes.
hiraditya added a commit: rG11a2b44ad412: add arcanist patch to fix arc diff.
bondhugula added a subscriber: bondhugula.May 28 2023, 12:48 PM
In D131699#3742559, @hiraditya wrote:

with arc diff

Deprecated: Function libxml_disable_entity_loader() is deprecated in /usr/share/arcanist/support/init/init-script.php on line 92
PHP Fatal error:  Uncaught Exception: Error while loading file "/usr/share/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /usr/share/arcanist/src/init/lib/PhutilBootloader.php:275
Stack trace:
#0 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#1 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#2 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#3 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 /usr/share/arcanist/src/filesystem/Filesystem.php(18): __phutil_autoload()
#5 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(247): include_once('...')
#6 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#7 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#8 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#9 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#10 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(97): __phutil_autoload()
#11 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(21): PhutilBootloader->registerLibrary()
#12 /usr/share/arcanist/src/init/init-library.php(70): PhutilBootloader::newLibrary()
#13 /usr/share/arcanist/support/init/init-script.php(96): require_once('...')
#14 /usr/share/arcanist/support/init/init-script.php(115): __arcanist_init_script__()
#15 /usr/share/arcanist/support/init/init-arcanist.php(3): require_once('...')
#16 /usr/share/arcanist/bin/arc(10): require_once('...')
#17 {main}
  thrown in /usr/share/arcanist/src/init/lib/PhutilBootloader.php on line 275

I still see the above error on Ubuntu 22.04 with PHP 2:8.1+92ubuntu1 and arcanist at 0fc22183e796fb8ac2e3a0a3f3f37aa964c6d7fa (Mar 30, 2023) and after patching with llvm/utils/phabricator/0001-Fix-PHP-8.1-incompatibility-with-arc-patch-D-12345.patch.

bondhugula added a comment.May 28 2023, 8:15 PM

More precisely, this is what I see:

arc diff
PHP Deprecated:  Function libxml_disable_entity_loader() is deprecated in /usr/share/arcanist/support/init/init-script.php on line 92

Deprecated: Function libxml_disable_entity_loader() is deprecated in /usr/share/arcanist/support/init/init-script.php on line 92
PHP Fatal error:  Uncaught Exception: Error while loading file "/usr/share/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /usr/share/arcanist/src/init/lib/PhutilBootloader.php:275
Stack trace:
#0 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#1 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#2 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#3 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 /usr/share/arcanist/src/filesystem/Filesystem.php(18): __phutil_autoload()
#5 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(247): include_once('...')
#6 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#7 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#8 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#9 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#10 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(97): __phutil_autoload()
#11 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(21): PhutilBootloader->registerLibrary()
#12 /usr/share/arcanist/src/init/init-library.php(70): PhutilBootloader::newLibrary()
#13 /usr/share/arcanist/support/init/init-script.php(96): require_once('...')
#14 /usr/share/arcanist/support/init/init-script.php(115): __arcanist_init_script__()
#15 /usr/share/arcanist/support/init/init-arcanist.php(3): require_once('...')
#16 /usr/share/arcanist/bin/arc(10): require_once('...')
#17 {main}
  thrown in /usr/share/arcanist/src/init/lib/PhutilBootloader.php on line 275

Fatal error: Uncaught Exception: Error while loading file "/usr/share/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /usr/share/arcanist/src/init/lib/PhutilBootloader.php:275
Stack trace:
#0 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#1 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#2 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#3 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 /usr/share/arcanist/src/filesystem/Filesystem.php(18): __phutil_autoload()
#5 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(247): include_once('...')
#6 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#7 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#8 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#9 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#10 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(97): __phutil_autoload()
#11 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(21): PhutilBootloader->registerLibrary()
#12 /usr/share/arcanist/src/init/init-library.php(70): PhutilBootloader::newLibrary()
#13 /usr/share/arcanist/support/init/init-script.php(96): require_once('...')
#14 /usr/share/arcanist/support/init/init-script.php(115): __arcanist_init_script__()
#15 /usr/share/arcanist/support/init/init-arcanist.php(3): require_once('...')
#16 /usr/share/arcanist/bin/arc(10): require_once('...')
#17 {main}
  thrown in /usr/share/arcanist/src/init/lib/PhutilBootloader.php on line 275
bondhugula added a comment.May 28 2023, 8:18 PM
In D131699#4378582, @bondhugula wrote:

More precisely, this is what I see:

arc diff
PHP Deprecated:  Function libxml_disable_entity_loader() is deprecated in /usr/share/arcanist/support/init/init-script.php on line 92

Deprecated: Function libxml_disable_entity_loader() is deprecated in /usr/share/arcanist/support/init/init-script.php on line 92
PHP Fatal error:  Uncaught Exception: Error while loading file "/usr/share/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /usr/share/arcanist/src/init/lib/PhutilBootloader.php:275
Stack trace:
#0 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#1 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#2 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#3 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 /usr/share/arcanist/src/filesystem/Filesystem.php(18): __phutil_autoload()
#5 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(247): include_once('...')
#6 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#7 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#8 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#9 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#10 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(97): __phutil_autoload()
#11 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(21): PhutilBootloader->registerLibrary()
#12 /usr/share/arcanist/src/init/init-library.php(70): PhutilBootloader::newLibrary()
#13 /usr/share/arcanist/support/init/init-script.php(96): require_once('...')
#14 /usr/share/arcanist/support/init/init-script.php(115): __arcanist_init_script__()
#15 /usr/share/arcanist/support/init/init-arcanist.php(3): require_once('...')
#16 /usr/share/arcanist/bin/arc(10): require_once('...')
#17 {main}
  thrown in /usr/share/arcanist/src/init/lib/PhutilBootloader.php on line 275

Fatal error: Uncaught Exception: Error while loading file "/usr/share/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /usr/share/arcanist/src/init/lib/PhutilBootloader.php:275
Stack trace:
#0 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#1 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#2 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#3 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#4 /usr/share/arcanist/src/filesystem/Filesystem.php(18): __phutil_autoload()
#5 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(247): include_once('...')
#6 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(207): PhutilBootloader->executeInclude()
#7 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(422): PhutilBootloader->loadLibrarySource()
#8 /usr/share/arcanist/src/symbols/PhutilSymbolLoader.php(277): PhutilSymbolLoader->loadSymbol()
#9 /usr/share/arcanist/src/init/init-library.php(23): PhutilSymbolLoader->selectAndLoadSymbols()
#10 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(97): __phutil_autoload()
#11 /usr/share/arcanist/src/init/lib/PhutilBootloader.php(21): PhutilBootloader->registerLibrary()
#12 /usr/share/arcanist/src/init/init-library.php(70): PhutilBootloader::newLibrary()
#13 /usr/share/arcanist/support/init/init-script.php(96): require_once('...')
#14 /usr/share/arcanist/support/init/init-script.php(115): __arcanist_init_script__()
#15 /usr/share/arcanist/support/init/init-arcanist.php(3): require_once('...')
#16 /usr/share/arcanist/bin/arc(10): require_once('...')
#17 {main}
  thrown in /usr/share/arcanist/src/init/lib/PhutilBootloader.php on line 275

Sorry, it turns out that it was using the arc installed on the system from the Ubuntu repos (0~git20200925-1) instead of the git version at 0fc22183e796fb8ac2e3a0a3f3f37aa964c6d7fa patched by Fix PHP 8.1 incompatibility with arc patch D<12345>``. The latter works fine.

Revision Contents

Diff 494034

llvm/utils/phabricator/0002-Fix-PHP-8.1.5-empty-strlen-with-arc-diff-verbatim.patch

  • This file was added.
From 3a6e789fe893fc8c855826994d044d17a281fd85 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinstitt@google.com>
Date: Thu, 11 Aug 2022 10:26:21 -0700
Subject: [PATCH] Fix PHP 8.1.5 empty strlen with <$ arc diff --verbatim>
Fixes the following observed error with PHP 8.1.5:
| EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
| arcanist(head=master, ref.master=680fe6d06e5a)
| #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2364]
| #1 ArcanistDiffWorkflow::getDiffOntoTargets() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2342]
| #2 ArcanistDiffWorkflow::updateOntoDiffProperty() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:409]
| #3 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
Link: https://secure.phabricator.com/book/phabcontrib/article/contributing_code/
Link: https://reviews.llvm.org/D129232#3634072
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
src/workflow/ArcanistDiffWorkflow.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php
index 5b8ff40c..1b717b23 100644
--- a/src/workflow/ArcanistDiffWorkflow.php
+++ b/src/workflow/ArcanistDiffWorkflow.php
@@ -2361,7 +2361,7 @@ EOTEXT
// If we track an upstream branch either directly or indirectly, use that.
$branch = $api->getBranchName();
- if (strlen($branch)) {
+ if (phutil_nonempty_string($branch) && strlen($branch)) {
$upstream_path = $api->getPathToUpstream($branch);
$remote_branch = $upstream_path->getRemoteBranchName();
if ($remote_branch !== null) {
--
2.37.1.559.g78731f0fdb-goog