[llvm-dev] Phabricator/Arcanist feedback
Dan Liew via llvm-dev
llvm-dev at lists.llvm.org
Sun Dec 27 21:24:54 PST 2015
Hi,
I recently tried reviewing/committing some of my code on
Phabricator/Arcanist for the first time and I noticed that the docs
[1] ask for feedback, so here it is!
Phabricator functions reasonably well and it is a lot easier to write
comments and respond to comments on particular parts of code as
opposed to the old way of copy and pasting patches into e-mails sent
to llvm-commits. Two minors annoyances:
* IIRC you have to scroll to the bottom of the page and click a submit
button to have comments that have just been written accepted. This is
really annoying and unintuitive, especially if the patch is large
which means lots and lots and lots of scrolling.
* I also have a personal preference for writing review comments in
markdown. Phabricator seems to use some other syntax [3] which is not
compatible. I'm very used to writing reviews on GitHub which uses
GitHub flavoured markdown so I kept accidentaly typing that into the
comment boxes.
I came up against a number of issues when it came to actually
committing code once it had been reviewed.
# TL;DR
I don't like the arcanist tool at all and we should have a documented
manual way of committing code reviewed on Phabricator.
# Long version
The docs [2] to seem to suggest the **only correct way** (i.e.
properly closes the phabriacator review) is to commit your change is
via the arcanist command line tool.
I had/have several problems with the arcanist tool.
* It is written in PHP (provokes a knee-jerk "kill it with fire"
reaction in me and likely others). IMHO not the most suitable language
for writing a command line tool. As the rest of the points below
demonstrate
* I suspect that most people don't have (and don't want to have) the
PHP runtime installed just to run a command line tool.
* The tool does not work out of box due PHP's default configuration
(5.6.16 on Arch Linux). I see warnings like this.
```
PHP Warning: ini_set(): open_basedir restriction in effect. File() is
not within the allowed path(s):
(/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) in
/home/dan/dev/libphutil/scripts/__init_scr
ipt__.php on line 52
```
See the attached ``arc.txt`` for the full output.
I had to remove the ``open_basedir`` setting (making my PHP install
less secure) in ``php.ini`` to get the tool to work.
IMHO we should have other documented method(s) of committing the code
(that still correctly closes the review in phabricator) without using
the arcanist tool. Perhaps the Phabricator web interface should have
an option to generate the correctly formatted commit message text
which can then be used when committed manually?
I'm aware phabricator is an external project so we are at the mercy of
the Phabricator devs but it seems reasonable to me request a command
line tool not written in PHP, e.g. written in something more sensible
for a lightweight client to a server application (e.g. Python, Perl,
Go, ...).
I also have issue with the commands shown in the docs for committing
the code, i.e.
```
arc patch D<Revision>
arc commit --revision D<Revision>
```
This has some implicit assumptions that aren't mentioned.
* ``arc commit`` only works if you're using svn. If you're working
with LLVM via "git svn" it doesn't work. You can to commit the usual
way (i.e. ``git svn dcommit``)
* Running the ``arc patch`` command assumes you are on a clean master
branch and want to apply a patch uploaded Phabricator. This is
pointless (and won't work) if you are already on a local branch that
already has the commit you want to commit to trunk.
I'm happy to write patches to improve the documentation here but I
would like some guidance on what the correct method of committing
without arcanist is.
[1] http://llvm.org/docs/Phabricator.html#status
[2] http://llvm.org/docs/Phabricator.html#committing-a-change
[3] https://secure.phabricator.com/book/phabricator/article/remarkup/
HTH,
Dan
-------------- next part --------------
PHP Warning: ini_set(): open_basedir restriction in effect. File() is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) in /home/dan/dev/libphutil/scripts/__init_script__.php on line 52
Warning: ini_set(): open_basedir restriction in effect. File() is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) in /home/dan/dev/libphutil/scripts/__init_script__.php on line 52
[2015-12-27 14:01:02] ERROR 2: file_exists(): open_basedir restriction in effect. File(/etc/arcconfig) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:923]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 file_exists(string) called at [<phutil>/src/filesystem/Filesystem.php:923]
#1 Filesystem::pathExists(string) called at [<arcanist>/src/configuration/ArcanistConfigurationManager.php:290]
#2 ArcanistConfigurationManager::readSystemArcConfig() called at [<arcanist>/scripts/arcanist.php:116]
[2015-12-27 14:01:02] ERROR 2: is_link(): open_basedir restriction in effect. File(/etc/arcconfig) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:923]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 is_link(string) called at [<phutil>/src/filesystem/Filesystem.php:923]
#1 Filesystem::pathExists(string) called at [<arcanist>/src/configuration/ArcanistConfigurationManager.php:290]
#2 ArcanistConfigurationManager::readSystemArcConfig() called at [<arcanist>/scripts/arcanist.php:116]
[2015-12-27 14:01:02] ERROR 2: is_link(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:828]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 is_link(string) called at [<phutil>/src/filesystem/Filesystem.php:828]
#1 Filesystem::resolvePath(string) called at [<phutil>/src/filesystem/Filesystem.php:744]
#2 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#3 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#4 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: realpath(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:835]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 realpath(string) called at [<phutil>/src/filesystem/Filesystem.php:835]
#1 Filesystem::resolvePath(string) called at [<phutil>/src/filesystem/Filesystem.php:744]
#2 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#3 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#4 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: realpath(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:852]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 realpath(string) called at [<phutil>/src/filesystem/Filesystem.php:852]
#1 Filesystem::resolvePath(string) called at [<phutil>/src/filesystem/Filesystem.php:744]
#2 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#3 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#4 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: is_link(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:749]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 is_link(string) called at [<phutil>/src/filesystem/Filesystem.php:749]
#1 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#2 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#3 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: is_link(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:828]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 is_link(string) called at [<phutil>/src/filesystem/Filesystem.php:828]
#1 Filesystem::resolvePath(string) called at [<phutil>/src/filesystem/FileList.php:35]
#2 FileList::__construct(array) called at [<phutil>/src/filesystem/Filesystem.php:755]
#3 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#4 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#5 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: realpath(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:835]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 realpath(string) called at [<phutil>/src/filesystem/Filesystem.php:835]
#1 Filesystem::resolvePath(string) called at [<phutil>/src/filesystem/FileList.php:35]
#2 FileList::__construct(array) called at [<phutil>/src/filesystem/Filesystem.php:755]
#3 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#4 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#5 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: realpath(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/Filesystem.php:852]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 realpath(string) called at [<phutil>/src/filesystem/Filesystem.php:852]
#1 Filesystem::resolvePath(string) called at [<phutil>/src/filesystem/FileList.php:35]
#2 FileList::__construct(array) called at [<phutil>/src/filesystem/Filesystem.php:755]
#3 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#4 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#5 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[2015-12-27 14:01:02] ERROR 2: is_dir(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) at [/home/dan/dev/libphutil/src/filesystem/FileList.php:36]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=master, ref.master=14765d36f83a)
#0 is_dir(string) called at [<phutil>/src/filesystem/FileList.php:36]
#1 FileList::__construct(array) called at [<phutil>/src/filesystem/Filesystem.php:755]
#2 Filesystem::walkToRoot(string) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:72]
#3 ArcanistWorkingCopyIdentity::newFromPathWithConfig(string, NULL) called at [<arcanist>/src/workingcopyidentity/ArcanistWorkingCopyIdentity.php:22]
#4 ArcanistWorkingCopyIdentity::newFromPath(string) called at [<arcanist>/scripts/arcanist.php:123]
[1mUsage Exception:[m `arc patch` is only supported under git, hg, svn.
More information about the llvm-dev
mailing list