Page MenuHomePhorge

Fix PhutilLibraryMapBuilder to call the right function in log()
ClosedPublic

Authored by roguelazer on Aug 3 2021, 18:14.

Details

Reviewers
eax
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rARC5407e5e5c67e: Fix PhutilLibraryMapBuilder to call the right function in log()
Summary

arc liberate has been broken for a while because the log function attempts to call fwrite (whose third argument is an integer length) instead of fprintf. I think maybe PHP 5 was more lenient about this? Anyway, this bug makes other development somewhat tricky.

The arcanist source in general seems to be split between fwrite(STDERR, $message."\n") and fprintf(STDERR, "%\n", $message); I decided to go with the latter.

This is sort of a test balloon on submitting diffs to Phorge. Please feel free to let me know if there's anything you'd like done differently!

Test Plan

Ran arc liberate to regenerate source maps and didn't get any errors

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Aug 3 2021, 18:14

The arcanist source in general seems to be split between fwrite(STDERR, $message."\n") and fprintf(STDERR, "%s\n", $message); I decided to go with the latter.

I think the latter format would be preferred as the formatting is more common elsewhere in the code, as opposed to concatenation.

This looks good to me - though it might be worth submitting this upstream on secure-dot.

This revision is now accepted and ready to land.Aug 7 2021, 20:07

FWIW, I can't land this myself since I don't have push access to any repos; I would appreciate it if someone would either land it (or give me such permissions, if that's the preference).

@roguelazer - added you to Trusted Contributors so you should be able to land now

With this change these log messages are being written out, even though the default of $this->quiet is true. I think this is because rebuild-map.php script unconditionally calls setQuiet() with the value it's passed from the command-line, however ArcanistLiberateWorkflow does not specify or pass in a quiet argument.

I think we should update ArcanistLiberateWorkflow to add a --verbose argument, which if not specified then we pass the --quiet argument to the rebuild-map.php script. This inverts the flag that the script is expecting but I think will provide better default behavior (does not log these additional status messages) but still allows them to be seen with a flag.