Page MenuHomePhabricator

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

Authored by cspeckmim on Sep 4 2021, 5:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 5, 3:47 AM
Unknown Object (File)
Feb 10 2024, 10:38 PM
Unknown Object (File)
Feb 7 2024, 10:01 AM
Unknown Object (File)
Feb 4 2024, 1:23 AM
Unknown Object (File)
Jan 31 2024, 8:18 AM
Unknown Object (File)
Jan 28 2024, 10:32 AM
Unknown Object (File)
Jan 15 2024, 5:18 PM
Unknown Object (File)
Jan 13 2024, 3:27 AM
Subscribers

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
Branch
liberate
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 25553
Build 35339: Run Core Tests
Build 35338: arc lint + arc unit

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.Sep 4 2021, 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.Sep 4 2021, 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.Sep 5 2021, 12:29 AM
src/moduleutils/PhutilLibraryMapBuilder.php
225

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.Sep 5 2021, 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.Sep 5 2021, 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.Sep 7 2021, 6:02 PM
cspeckmim edited the summary of this revision. (Show Details)