Update December 1: I have disabled Harbormaster build plan on Phabricator and will remove supporting infrastracture in the next few days.
Hi! While phabricator will stay up until 1 October 2024 (Update on GitHub pull requests) and existing reviews receive updates I propose to disable pre-merge checks for all phabricator reviews.
Why:
That will allow us to make changes to Pull Request builds that are somewhat compicated by the need to support both systems. From the top of my head: moving to GitHub Actions, reorganizing bulid pipelines and merge “generic” and “libcxx” pipelines.
Currently only about ~2% of all pre-merge build requests are coming from Phabricator.
When: what about 1 December? As I understand there is no need to optimize for a long tail of in-progress reviews as they might last for few more months.
That will allow us to make changes to Pull Request builds that are somewhat compicated by the need to support both systems. From the top of my head: moving to GitHub Actions, reorganizing bulid pipelines and merge “generic” and “libcxx” pipelines.
If dropping presubmit testing for Phabricator can simplify and improve the presubmit testing experience on pull requests, that definitely sounds like the right tradeoff to me!
I’d like to keep it around for a few more weeks, maybe until December 1st? From the libc++ point of view, dropping Phab CI is equivalent to saying that all remaining patches need to be migrated, cause we can’t do a review without our pre-commit CI being triggered.
One could always have the reviewer post a dummy PR with the changes just to test it in CI, but keep the discussion in Phabricator with all the existing context. A bit ugly but it at least avoids having two discussion threads…
Personally at this point I think I’d request any Phab review to be moved over. Either way kinda sucks but I feel like it’s easier to just migrate a code review. And if a code review is that huge that it can’t be moved over, perhaps it needs to be split up to be human manageable. Just my .02, folks are free to do however they want but for libc++ I think it would make sense to migrate in most cases. We don’t have many active Phab reviews left anyway.
I’m in favor of disabling premerge checks for Phabricator uploads.
You’re probably already aware, but you can no longer create new Phabricator reviews, it’s just old patches now. If those old reviews need premerge check results, uploading a PR on the side is one way to get that validation, as @jrtc27 says.