Rebase arcanist for on phorge #15
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Updated .arcconfig to we.phorge.it Refs T15006 I would also note, I created this revision using the container setup developed by @willson556 mentioned in T15011#370 and it worked great! Test Plan: Ran `arc install-certificate` and was able to get token from the right place and install it Ran `arc which` saw that it identified `we.phorge.it` Reviewers: speck, avivey, tobiaswiese, O1 Blessed Committers, eax, #blessed_committers Reviewed By: speck, avivey, tobiaswiese, O1 Blessed Committers, eax, #blessed_committers Subscribers: deadalnix, chris, willson556, speck Maniphest Tasks: T15006 Differential Revision: https://we.phorge.it/D25003
…or for Arcanist. Test Plan: Generated Diviner documentation on a local install and verified that the changes look good. Reviewers: O1 Blessed Committers, deadalnix Reviewed By: O1 Blessed Committers, deadalnix Subscribers: speck, tobiaswiese Maniphest Tasks: T15012 Differential Revision: https://we.phorge.it/D25008
Test Plan: Looked at the rendered markdown. Reviewers: O1 Blessed Committers, 20after4 Reviewed By: O1 Blessed Committers, 20after4 Subscribers: 20after4, eax, speck, tobiaswiese Maniphest Tasks: T15006 Differential Revision: https://we.phorge.it/D25009
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 Reviewers: O1 Blessed Committers, eax Reviewed By: O1 Blessed Committers, eax Subscribers: chris, speck, tobiaswiese Differential Revision: https://we.phorge.it/D25017
Summary: Author field is formatted with csprintf, which would be appropriate if the resulting string was concatenated into a shell command as a string -- but because the flags are passed as a vector of strings and not parsed by the shell, this results in extraneous shell quoting making it into to author field. In particular this renders my name as D'\''Anna instead of D'Anna Test Plan: Performed 'arc patch' with and without these changes, confirmed that my apostrophe was no longer mangled by shell quotes in the resulting commit. Reviewers: O1 Blessed Committers, speck Reviewed By: O1 Blessed Committers, speck Subscribers: MacFan4000, Ekubischta, speck, tobiaswiese, valerio.bozzolan Differential Revision: https://we.phorge.it/D25026
Summary: Ref T13630. Piledriver cares about EBS error specifics; expand this class a bit to expose more error information. Test Plan: Created and destroyed resource piles with Piledriver control code, elsewhere. Maniphest Tasks: T13630 Differential Revision: https://secure.phabricator.com/D21732
Summary: Ref T13653. This entirely exists to support converging into the right swap state in deployment workflows. Test Plan: Ran unit tests. Maniphest Tasks: T13653 Differential Revision: https://secure.phabricator.com/D21733
Summary: This default CA bundle file hasn't been updated since 2016. Update it to the current cURL extraction. I believe this is notably impactful because of a new "Let's Encrypt" certificate, but didn't hunt down the particulars. Test Plan: Confirmed the hash matches the published hash: ``` $ openssl dgst -sha256 resources/ssl/default.pem SHA256(resources/ssl/default.pem)= ae31ecb3c6e9ff3154cb7a55f017090448f88482f0e94ac927c0c67a1f33b9cf ``` This assurance is fairly meaningless since both the hash and file are published on `curl.se`. It didn't get corrupted by stellar radiation before it made it into Git, at least? Differential Revision: https://secure.phabricator.com/D21739
Summary: Ref T13588. See that task for discussion. Improve behavior under PHP8.1, particularly the deprecation warning raised by calling `strlen(null)`. Test Plan: - Ran `arc help`, `arc branches`, `arc diff`, etc., under PHP 8.1 and PHP 7.4. - Created this change with PHP8.1. Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21740
Summary:
Ref T13588. "each()" has been a bad idea for a long time, and was formally deprecated in PHP 7.2 and removed in PHP 8.0.
Catch use of "each()" in lint and reject it.
Test Plan:
Added and ran unit tests.
{F10021268}
Subscribers: cspeckmim
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21741
Summary: Ref T13588. I pointed my local `php` at PHP 8.1 and this is what I've hit so far; all these cases seem very unlikely to have any subtle behavior. Test Plan: Ran various `arc` workflows under PHP 8.1. Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21742
Summary: Updated the mercurial script to pull the `parseurl` function from the new module if pulling from the old module fails. Also updated pulling of `remoteopts` to follow the same pattern of fallback. Fixes T13672 Test Plan: Using mercurial 6.1 I ran `arc patch Dnnnnn --trace` and verified that it succeeded and in the trace output it used the `arc-ls-markers` extension. Using mercurial 4.7 I ran `arc patch Dnnnnn --trace` and verified that it succeeded and in the trace output it used the `arc-ls-markers` extension. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T13672 Differential Revision: https://secure.phabricator.com/D21747
Summary: Ref T13588. The lint renderer has some "strlen(null)" issues; correct them. Test Plan: Hit lint messages under PHP 8.1. Reviewers: 0 Subscribers: jeremy.norris, vostok4, vhbit, jaydiablo, 0 Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21743
Summary: Ref T13588. Test Plan: Ran unit tests in D21757. Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21758
…e multiple types Summary: Ref T13588. Adds "phutil_nonempty_string()" and similar methods to support PHP8.1 compatibility. Test Plan: Added unit tests, ran unit tests. Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21762
Summary: Ref T13658. One challenge in forking Phabricator is product name references in user-visible strings, particularly in "pht()". Add a linter to identify the use of product name literals in "pht()" text. The linter could fix these automatically, but it looks like there are fewer than 200 across both Arcanist and Phabricator and some sampling suggests that many are probably clearer when rewritten into generic language anyway. Test Plan: Ran linter, saw it pop out reasonable warnings. Maniphest Tasks: T13658 Differential Revision: https://secure.phabricator.com/D21763
Summary: Ref T13658. Remove all product name literals from "pht()" strings, by replacing them with generic text where that feels reasonably natural, or "PlatformSymbols" calls elsewhere. These calls were identified with `arc lint --everything` after enabling the lint rule in D21763. Test Plan: Read strings, ran "arc". Maniphest Tasks: T13658 Differential Revision: https://secure.phabricator.com/D21764
…st product names Summary: Ref T13658. The lint rule called "getStringLiteralValue()", which produces string literals for fewer nodes than "evalStatic()". Switch to "evalStatic()", then fix new warnings. Test Plan: This test plan is non-exhaustive. - Ran "arc lint --everything --output summary" to generate new warnings. Maniphest Tasks: T13658 Differential Revision: https://secure.phabricator.com/D21776
Summary:
Ref T13675. Ref T13556. The "STDOUT" and "STDERR" constants are defined by the PHP CLI SAPI, in `cli_register_file_handles()`.
The "native arc" embedded PHP wrapper doesn't define these, and there's no real reason to define them, since they're just defined in terms of the PHP stream wrappers ("php://stdin", etc) anyway.
This patch isn't exhaustive (and a subsequent change should add lint, rejecting these magic constants) but is just trying to make native `arc` functional.
Test Plan: Created this revision with a standalone native `arc` binary.
Subscribers: cspeckmim
Maniphest Tasks: T13675, T13556
Differential Revision: https://secure.phabricator.com/D21794
Summary: Ref T13676. Ref T13588. These properties on PhutilURI have flexible types, just wrap them. Test Plan: Built a new Drydock working copy with an HTTPS URI under PHP 8.1, see T13676. Maniphest Tasks: T13676, T13588 Differential Revision: https://secure.phabricator.com/D21798
Summary: Ref T13676. With increased PHP8.1 strictness around null, it seems reasonable to add some convenience parsing to "PhutilArgumentParser". Add a helper function for parsing integers. Test Plan: See next change. Tried these arguments: ``` --count '' --count asdf --count 0 --count -3 --count 999999999999999999999999999999999999999999999234 --count 5 ``` Got sensible parsing and appropriate feedback in all cases. Maniphest Tasks: T13676 Differential Revision: https://secure.phabricator.com/D21799
Summary: Ref T13675. When a process daemonizes, it needs to close these actual file descriptors, so the calling context must provide them. Elsewhere, modify the embedded "arc" to provide them. Then use them in place of reopening the streams. Test Plan: Ran locally with embedded "arc", will deploy for production hangs. Maniphest Tasks: T13675 Differential Revision: https://secure.phabricator.com/D21804
Summary: Ref T13603. This is just a small piece of cleanup I've wanted to do for a while: different languages might have different list separators and repeating this implosion manually all over the place is a bit ugly even if the beahvior is never a function of translation language. Test Plan: See next change. Maniphest Tasks: T13603 Differential Revision: https://secure.phabricator.com/D21814
Summary: Ref T13588. Additional PhutilURI fixes for PHP 8.1. Test Plan: Ran "arc unit --everything" in Phabricator. Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21822
Summary: Ref T13588. "arc-ls-markers" emits a "branch-state" marker so callers can identify which branch is active in the working copy. This marker doesn't have an associated commit, so trying to generate a display name fails under stricter PHP 8.1 rules when we try to `substr(null, ...)`. Don't attempt to generate a display name for markers with no commit hash. Test Plan: - Ran `arc branches` under PHP 8.1 in a Mercurial repository. - Before: fatal. - After: sensible output. Maniphest Tasks: T13588 Differential Revision: https://secure.phabricator.com/D21825
Summary: Use the name "Phorge" as the defined platform. Also prepare to rename the core library "phorge" rather then "phabricator" - see next diff. T15006 Test Plan: Deployed change, tooltip for "Config" shows "Configure Phorge" Reviewers: O1 Blessed Committers, speck Reviewed By: O1 Blessed Committers, speck Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew Maniphest Tasks: T15006 Differential Revision: https://we.phorge.it/D25046
Summary: api doc : https://dev.twitch.tv/docs/api/reference oauth2 doc : https://dev.twitch.tv/docs/authentication Test Plan: I have successfully setup OAuth2 authentication against Twitch Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno Tags: #auth Maniphest Tasks: T15122 Differential Revision: https://we.phorge.it/D25056
* Add error definition * Validate if force push can be done
…iff (#38) * Add --rebase flag for arc diff to enable rebase before creating the diff * make runRebaseToStable a seperate function * update prompt when rebase failed * update prompt when rebase failed
…res or ongoing builds
…res or ongoing builds
Summary: Collect data at `arc diff` (not only the first one) for DevX KPI. https://docs.google.com/document/d/1oe0hNo0BxAoe6tBxiVKVuniqfczWkHvQ-40HJUPDnx8/edit# https://docs.google.com/document/d/1ehLeRTGD5ftgcM7hb9gc-eD9VnD_QjxTM7UJoqH0alI/edit# Test Plan: payload to curl: (updated to use ms for timestamps) ``` { "event_type": "dev_branch_info", "from": "arc", "events": [ { "event_ts": 1639594695870, "revision_id": "321035", "code_review_type": "phab", "branch_create_ts": 1638921535000, "arc_diff_ts": 1639594688856, "base_commit_sha": "13d3a3c3b100979c34dda261fe21253e3571bc46", "base_commit_ts": 1638485776000, "head_commit_sha": "d8b62102ce6c341f7acd0db324b3ea07e41c6418", "head_commit_ts": 1639594674000, "author": "jason.wang", "branch_name": "arc_data_new", "host_name": "jason-wang--C02G7BQPMD6R" } ] } ``` checking with `$info = curl_getinfo($ch);`, we saw successful return code 201 printed message and exception handling: {F5556198} {F5556201} Reviewers: tianxiang.chen, boyang.tian, #devx Reviewed By: tianxiang.chen, #devx Differential Revision: https://phabricator.robinhood.com/D321035
Summary: per title Test Plan: Set flag for arcanist repo to test. Reviewers: #devx, tianxiang.chen, boyang.tian Reviewed By: #devx, tianxiang.chen Differential Revision: https://phabricator.robinhood.com/D324248
Summary: We changed the interface in devhooks. Test Plan: none Reviewers: boyang.tian Reviewed By: boyang.tian Differential Revision: https://phabricator.robinhood.com/D325638
Summary: as title Test Plan: none Reviewers: boyang.tian Reviewed By: boyang.tian Differential Revision: https://phabricator.robinhood.com/D326615
Summary: In the first arc diff, the existing variable $result_id is not string. Cast it to string to ensure a string is sent as revision_id. Also log the content for easier debugging.
Test Plan: {F5650736}
Reviewers: tianxiang.chen, boyang.tian
Reviewed By: tianxiang.chen
Differential Revision: https://phabricator.robinhood.com/D327706
Summary: Failed to send arc metrics data to devhooks due to `Undefined offset: 1` when using `sw diff`. https://hood.slack.com/archives/G01GBQWJGKU/p1641263947017100 This is caused by sherwood calling `arc diff` during a cascading rebase and as head is detached the original `getBranchName` function failed to get the correct branch name. Fix the issue by making `getBranchName` to handle the case during a rebase. SW-ID-079b8a9be9 Test Plan: `sw diff` now successfully sends metrics to devhooks. Reviewers: tianxiang.chen, boyang.tian Reviewed By: tianxiang.chen Differential Revision: https://phabricator.robinhood.com/D329294
Summary: improved curl error handling. print full exception on error for easier debugging. Test Plan: local testing Reviewers: paul.tarjan, boyang.tian, jacqueline.lee, #devx, david.aghassi Reviewed By: #devx, david.aghassi Differential Revision: https://phabricator.robinhood.com/D341312
Summary: https://hood.slack.com/archives/G01GBQWJGKU/p1644453734640829 The root cause was: ``` git reflog --date=unix --pretty='%gd' flink fatal: ambiguous argument 'flink': both revision and filename Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' ``` Because there's file/folder named flink etc. Test Plan: local tests: submit diffs from local branches with same names as some file (like s3cret, next, ...) Reviewers: #devx, jacqueline.lee, paul.tarjan, boyang.tian Reviewed By: #devx, jacqueline.lee Differential Revision: https://phabricator.robinhood.com/D343403
Summary: use the more accurate author time for commits Test Plan: none Reviewers: boyang.tian, gabriel.silk, brian.king, warren.smith Reviewed By: brian.king Differential Revision: https://phabricator.robinhood.com/D351558
* bug fix * bug fix
…ocked Commit to address this SEV action: https://robinhood.atlassian.net/browse/SCA-5827 Change error message in the following case: * Someone has added ALLOW_FAILED_TEST * They try to land, but are blocked because tests are ongoing With this diff, what should now happen in this case is that the user will get the following error message: Blocking ongoing build plans on D%s are fatal for land (build plan %s: %s). If you are in a SEV and must land this quickly, you can replace ALLOW_FAILED_TESTS with ALLOW_ONGOING_TESTS; otherwise, please wait until the ongoing build plans have completed before trying to land again
|
lgtm |
|
@dkhenry What is the motivation for this change? I think rebasing onto a fork requires further scrutiny and slow rollout before landing. I suspect the motivation is as mentioned in the description, being able to use php8? If so, lets cherry-pick that one over by itself, unless there are features/fixes we want in phorge |
|
@joeljeske this is entirely so we can use php8. I initially tried to just spot fix our fork to work on php8, but ran into a lot of problems which is when I investigated rebasing off upstream. The current upstream philicity/arcanist is no longer supported, but development has moved to phorge. Even with that I needed 86f3341 in addition to the rebase to get it working on 8.2 which is the current php version. |
dkhenry
force-pushed
the
koz-rebase-phorge
branch
from
86f3341 to
2ff565a
Compare
- Remove depricated utf8_decode function call - Explicit cast to int for result of division - returned result from differential.query appears to have changed to array's and not keyed objects. Patch needs to search the array
dkhenry
force-pushed
the
koz-rebase-phorge
branch
from
2ff565a to
af49388
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rebase our fork of arcanist off upstream.The new upstream for this project is https://we.phorge.it/source/arcanist.git. This also adds in one extra commit to remove a call to utf8_decode, this function was deprecated as of php 8.2