[llvm-dev] Phabricator down for maintenance tonight (original) (raw)

Tom Stellard via llvm-dev llvm-dev at lists.llvm.org
Tue Jul 28 16:35:16 PDT 2020

On 7/28/20 6:58 PM, Mehdi AMINI wrote:

+Tom Stellard <mailto:tstellar at redhat.com>  : can we get a fork of https://github.com/phacility/phabricator in the LLVM project org on GitHub in order to host our patched version and start collaboratively maintaining it?

Sure, I've cloned this and added you as an admin for the team, so you can grant commit access.

-Tom

-- Mehdi

On Tue, Jul 28, 2020 at 12:49 PM MyDeveloper Day <mydeveloperday at gmail.com <mailto:mydeveloperday at gmail.com>> wrote: Could we ever consider adding https://github.com/r4nt/phabricator/tree/llvm-production as a new read/only observe Diffusion repository in reviews.llvm.org <http://reviews.llvm.org>? I'd be happy to do code reviews? MyDeveloperDay On Tue, Jul 28, 2020 at 8:46 PM MyDeveloper Day <mydeveloperday at gmail.com <mailto:mydeveloperday at gmail.com>> wrote: Awesome, thanks for sharing Here is a patch (based off yours) but this adds the "Collapse" button back in, remember to rerun  phabricator/bin/celerity map MyDeveloperDay diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index fdaa99a..9b18031 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -214,6 +214,23 @@ final class PHUIDiffInlineCommentDetailView  (!$issynthetic);  if ($canreply) { +      $actionbuttons[] = id(new PHUIButtonView()) +        ->setTag('a') +        ->setIcon('fa-reply') +        ->setTooltip(pht('Reply')) +        ->addSigil('differential-inline-reply') +        ->setMustCapture(true) +        ->setAuralLabel(pht('Reply')); + +      $actionbuttons[] = id(new PHUIButtonView()) +        ->setTag('a') +        ->setIcon('fa-times') +        ->setTooltip(pht('Collapse')) +        ->addSigil('differential-inline-collapse') +        ->setMustCapture(true) +        ->setAuralLabel(pht('Collapse')); + +  $menuitems[] = array(  'label' => pht('Reply to Comment'),  'icon' => 'fa-reply', diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 7293f89..bb84997 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -2402,6 +2402,19 @@ JX.install('DiffChangesetList', {  ['differential-inline-comment', 'inline-action-dropdown'],  onmenu); +     // restore button for replying to comments +        var onreply = JX.bind(this, this.onInlineEvent, 'reply'); +        JX.Stratcom.listen( +              'click', +              ['differential-inline-comment', 'differential-inline-reply'], +                                          onreply); +        var oncollapse = JX.bind(this, this.onInlineEvent, 'collapse'); +        JX.Stratcom.listen( +              'click', +              ['differential-inline-comment', 'differential-inline-collapse'], +                                          oncollapse); +     // END +  var ondraft = JX.bind(this, this.onInlineEvent, 'draft');  JX.Stratcom.listen(  'keydown', @@ -2491,6 +2504,12 @@ JX.install('DiffChangesetList', {  case 'delete':  inline.delete(isref);  break; +        case 'reply': +          inline.reply(); +          break; +        case 'collapse': +          inline.setCollapsed(!inline.isCollapsed()); +          break;  case 'draft':  inline.triggerDraft();  break; On Tue, Jul 28, 2020 at 8:02 PM Mehdi AMINI <joker.eph at gmail.com_ _<mailto:joker.eph at gmail.com>> wrote: On Tue, Jul 28, 2020 at 11:04 AM MyDeveloper Day <mydeveloperday at gmail.com <mailto:mydeveloperday at gmail.com>> wrote: Out of interest are you keeping the local modifications in a fork of the Phabricator source (llvm-phabricator)? Firstly we should be keeping any changes we make in source control but also it's good to review those changes. I didn't innovate, Manuel set it up originally and I just reused the flow there: https://github.com/r4nt/phabricator/tree/llvm-production That said our "pre-production" setup isn't super streamlined, I'd like to improve this in the future. Let me know if you have suggestions :) I have found over the years that I've had to redo some of my local modifications as @evan changes the underlying infrastructure, nothing major but sometimes can cause a little downtime when doing an upgrade. Yeah we had some merge conflicts as well, I reduced some of our diff with upstream in the process, I think we can reduce a bit more. What I did for this upgrade is that 10 days ago I: - duplicate the entire VM and the database on a new machine. - merged the latest stable version and went through the conflicts - upgraded the database (had to manually craft some SQL queries as some duplicated records were breaking newly added keys). - tested manually this cloned instance by opening fake reviews. Had to fix the mail config since they changed the way it is configured. - Had to patch some broken code in phab in the SendGrid interaction with respect to attachments: https://discourse.phabricator-community.org/t/sendgrid-mailer-metamta-differential-attach-patches-true-errors-with-the-attachment-type-cannot-contain-or-crlf-characters/4098 Then when everything was working, I took down the production instance and repeated the migration steps (except fixing the merge conflicts and the patching that I had kept in git). -- Mehdi

MyDeveloperDay On Tue, Jul 28, 2020 at 6:17 PM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org_ _<mailto:llvm-dev at lists.llvm.org>> wrote: On Tue, Jul 28, 2020 at 4:25 AM James Henderson <jh7370.2008 at my.bristol.ac.uk_ _<mailto:jh7370.2008 at my.bristol.ac.uk>> wrote: Thanks for the work too! Not specifically a regression, but since the upgrade, I find it annoying now that when I want to do something in relation to an inline comment (collapse it, reply to it etc), I now have to click on a drop-down menu in the comment header bar to select the option, whereas before the icons were inline there before. Is this something that can be easily addressed? Seems like https://secure.phabricator.com/D21244 <https://secure.phabricator.com/D21244>I patched back the reply button, collapse seems a bit more intrusive. That said the keyboard shortcuts are pretty nice: you can click on a comment to select it, n for selecting the next one, p the previous one, q to collapse, r to reply. -- Mehdi James On Tue, 28 Jul 2020 at 05:56, David Blaikie via llvm-dev <llvm-dev at lists.llvm.org_ _<mailto:llvm-dev at lists.llvm.org>> wrote: Thanks for the work! On Mon, Jul 27, 2020 at 9:53 PM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org_ _<mailto:llvm-dev at lists.llvm.org>> wrote: > > Hi, > > Phab is upgraded now. It was ~2 years since the last upgrade so we got a few features along the way. > > We may have regressed some aspects as well, I only tested the basic functionalities. > Let me know if you see anything behaving unexpectedly! > > -- > Mehdi > > > On Mon, Jul 27, 2020 at 7:53 PM Mehdi AMINI <joker.eph at gmail.com_ _<mailto:joker.eph at gmail.com>> wrote: >> >> Hi, >> >> FYI, I'm taking Phabricator down for an upgrade right now. >> >> -- >> Mehdi >> >

> LLVM Developers mailing list > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

LLVM Developers mailing list llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

LLVM Developers mailing list llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

More information about the llvm-dev mailing list