Page MenuHomePhabricator
Differential D21718

Update "arc liberate" to fix error with PHP 8, remove logging, modify error handling
ClosedPublic
Actions

Authored by cspeckmim on Sep 4 2021, 5:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 5:38 PM
Unknown Object (File)
Thu, Nov 14, 4:03 AM
Unknown Object (File)
Tue, Nov 12, 5:29 AM
Unknown Object (File)
Mon, Nov 11, 7:59 AM
Unknown Object (File)
Mon, Nov 11, 7:59 AM
Unknown Object (File)
Mon, Nov 11, 7:35 AM
Unknown Object (File)
Sat, Nov 9, 4:47 PM
Unknown Object (File)
Thu, Nov 7, 6:40 PM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
rARCcd17e8441214: Update "arc liberate" to fix error with PHP 8 and add "--verbose" argument to…
Summary

Reported at the phorge project (https://we.phorge.it/D25017), running arc liberate fails on PHP 8 due to the log() function using fwrite() incorrectly assuming a format pattern can be used.

This updates to remove most of these status messages are they are largely uninformative and instead we can report progress.

  • Remove the --quiet argument
  • Always display the progress
  • Remove all informational/status log statements
  • Update error handling based on the exit code of the rebuild-map.php script
Test Plan

Tested using both PHP 7.3 and PHP 8:

  1. I ran arc liberate and saw the standard output:
 SCAN  Searching for libraries in the current working directory...
 WORK  Updating library: src/
Done.
 DONE  Updated library.
  1. I ran deleted phabricator/src/.phutil_module_cache and ran arc liberate /src, verifying that progress was displayed while the map was computed.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cspeckmim created this revision.Sep 4 2021, 5:22 PM
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptSep 4 2021, 5:22 PM
Herald added a subscriber: epriestley. · View Herald Transcript
Harbormaster completed remote builds in B25540: Diff 51742.Sep 4 2021, 5:22 PM
cspeckmim requested review of this revision.Sep 4 2021, 5:22 PM
cspeckmim retitled this revision from Update "arc liberate" logging and add "--verbose" argument to adjust it to Update "arc liberate" to fix error with PHP 8 and add "--verbose" argument to adjust it.Sep 4 2021, 5:23 PM
epriestley accepted this revision.Sep 4 2021, 9:35 PM

I'd favor this change:

  • Remove --quiet.
  • Don't add --verbose.
  • Remove the log() method and all calls to it.

My argument is:

  • I think it's a pretty reasonable claim that all the log() messages are useless noise.
  • ...so adding a new --verbose flag just for these messages seems a tad silly.
  • I think the only useful thing is the progress bar that shows up if the thing is taking a while (run rm src/.phutil_module_cache; arc liberate src/ in Phabricator to see it -- seems nice-to-have for first-time execution in phabricator/), which this change hides by default; this seems slightly worse than the status quo.
  • Best behavior seems like "never show messages, always show progress bar".

But this is all super minor and I'm fine with this change as-is, too.

This revision is now accepted and ready to land.Sep 4 2021, 9:35 PM
Herald added a subscriber: Korvin. · View Herald TranscriptSep 4 2021, 9:35 PM
cspeckmim planned changes to this revision.Sep 4 2021, 9:38 PM

I'll do that

cspeckmim updated this revision to Diff 51755.Sep 5 2021, 12:29 AM

Change approach - remove --verbose, remove --quiet, remove most instances of log() being used, keep the instance when an error is encountered.

This revision is now accepted and ready to land.Sep 5 2021, 12:29 AM
Harbormaster completed remote builds in B25553: Diff 51755.Sep 5 2021, 12:30 AM
cspeckmim added inline comments.Sep 5 2021, 12:31 AM
src/moduleutils/PhutilLibraryMapBuilder.php
208–209

This is the only case where a log is written out in case of error. There's another Filesystem::writeFile() in writeLibraryMap() and isn't in a try/catch. Maybe this try/catch should be removed? I think removing it will result in rebuild-map.php exiting with a non-zero exit code which should propagate properly I think.

cspeckmim requested review of this revision.Sep 5 2021, 12:36 AM
cspeckmim edited the summary of this revision. (Show Details)
cspeckmim edited the test plan for this revision. (Show Details)
epriestley added a comment.EditedSep 5 2021, 12:38 AM

I think it's reasonable to remove the try/catch and just let the exception escape. It (the try-catch + log) isn't consistent with the approach to error handling in the rest of the codebase, and I can't think of any valid reason to continue here after a write failure.

If someone crawls out of the woodwork with an amazing use case involving a partially read-only disk that needs to rebuild maps in production or something (?????), we could add a flag like --do-not-write-the-cache.

cspeckmim updated this revision to Diff 51757.Sep 5 2021, 12:40 AM

Remove log() entirely, let filesystem errors escape outwardly.

Harbormaster completed remote builds in B25555: Diff 51757.Sep 5 2021, 12:40 AM
epriestley accepted this revision.Sep 5 2021, 12:45 AM

Thanks!

This revision is now accepted and ready to land.Sep 5 2021, 12:45 AM
cspeckmim updated this revision to Diff 51759.Sep 5 2021, 1:02 AM

Update error handling of ArcanistLiberateWorkflow to detect failure from running rebuild-map.php in a separate process.

Harbormaster completed remote builds in B25557: Diff 51759.Sep 5 2021, 1:03 AM
cspeckmim requested review of this revision.Sep 5 2021, 1:06 AM

Sorry I made a side-stop checking out the error-handling behavior from ArcanistLiberateWorkflow - I made some updates so it can detect the failure to avoid indicating success.

Inserting an exception into PhutilLibraryMapBuilder::writeSymbolCache() I get this:

Screen Shot 2021-09-04 at 9.05.15 PM.png (348×1 px, 134 KB)

Screen Shot 2021-09-04 at 9.04.58 PM.png (792×1 px, 221 KB)

The normal arc liberate without intentional error still behaves the same

cspeckmim added a comment.Sep 5 2021, 1:08 AM

Err, I updated the error message after running those cases so it no longer says "Failed liberating:" but "Failed to update library:"

epriestley accepted this revision.Sep 5 2021, 1:16 AM
This revision is now accepted and ready to land.Sep 5 2021, 1:16 AM
Closed by commit rARCcd17e8441214: Update "arc liberate" to fix error with PHP 8 and add "--verbose" argument to… (authored by cspeckmim). · Explain WhySep 5 2021, 1:22 AM
This revision was automatically updated to reflect the committed changes.
cspeckmim added a commit: rARCcd17e8441214: Update "arc liberate" to fix error with PHP 8 and add "--verbose" argument to….
cspeckmim retitled this revision from Update "arc liberate" to fix error with PHP 8 and add "--verbose" argument to adjust it to Update "arc liberate" to fix error with PHP 8, remove logging, modify error handling.Sep 7 2021, 6:02 PM
cspeckmim edited the summary of this revision. (Show Details)

Revision Contents
Changeset List

PathSize
src/
moduleutils/
54 lines
workflow/
38 lines
support/
lib/
5 lines

Diff 51761

View Options

src/moduleutils/PhutilLibraryMapBuilder.php

Loading...
View Options

src/workflow/ArcanistLiberateWorkflow.php

Loading...
View Options

support/lib/rebuild-map.php

Loading...
Phabricator · Built by Phacility