Page MenuHomePhabricator

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

Authored by cspeckmim on Sat, Sep 4, 5:22 PM.

Details

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 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.Sat, Sep 4, 5:23 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.Sat, Sep 4, 9:35 PM

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.Sun, Sep 5, 12:29 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 edited the summary of this revision. (Show Details)
cspeckmim edited the test plan for this revision. (Show Details)

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.

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

This revision is now accepted and ready to land.Sun, Sep 5, 12:45 AM

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

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

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

This revision is now accepted and ready to land.Sun, Sep 5, 1:16 AM
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.Tue, Sep 7, 6:02 PM
cspeckmim edited the summary of this revision. (Show Details)