This is an archive of the discontinued LLVM Phabricator instance.

[Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration
ClosedPublic

Authored by pscoro on Jul 10 2023, 10:42 AM.

Details

Summary

See discourse thread https://discourse.llvm.org/t/rfc-support-cmake-option-to-control-link-type-built-for-flang-runtime-libraries/71602/18 for full details.

Flang-rt is the new library target for the flang runtime libraries. It builds the Flang-rt library (which contains the sources of FortranRuntime and FortranDecimal) and the Fortran_main library. See documentation in this patch for detailed description (flang-rt/docs/GettingStarted.md).

This patch aims to:

  • integrate Flang's runtime into existing llvm infrasturcture so that Flang's runtime can be built similarly to other runtimes via the runtimes target or via the llvm target as an enabled runtime
  • decouple the FortranDecimal library sources that were used by both compiler and runtime so that different build configurations can be applied for compiler vs runtime
  • add support for running flang-rt testsuites, which were created by migrating relevant tests from flang/test and flang/unittest to flang-rt/test and flang-rt/unittest, using a new check-flang-rt target.
  • provide documentation on how to build and use the new FlangRT runtime

Diff Detail

Event Timeline

pscoro created this revision.Jul 10 2023, 10:42 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
pscoro requested review of this revision.Jul 10 2023, 10:42 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
pscoro edited the summary of this revision. (Show Details)Jul 10 2023, 10:49 AM
pscoro removed reviewers: Restricted Project, Restricted Project, Restricted Project.
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 10 2023, 10:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
pscoro added inline comments.Jul 10 2023, 10:58 AM
flang/cmake/modules/AddFlang.cmake
68

This is something I am still investigating.

PIC is needed for the object libraries if FlangRT is being built as shared. I am thinking I will add a condition for this to only be enabled for FortranRuntime and FortranDecimalRT. Also, if my understanding is correct, if we build FlangRT as static this change may also not be needed (and keeping it may be slightly less optimal?), so I may ad a condition for this as well.

My understanding of PIC is limited so if anybody sees any problems with my understanding please let me know.

flang/include/flang/Runtime/float128.h
42 ↗(On Diff #538736)

Entirely uncertain why this code was giving me issues during my testing. I commented it out for the time being but I will take a closer look later. I hope that it is related to my setup and not something about the patch itself, however I will have to investigate further before I can draw any conclusions.

flang/lib/Decimal/CMakeLists.txt
62

The code in AddFlang.cmake seems to build object libraries for calls to add_flang_library by default when STATIC argument is passed.

flang/runtime/CMakeLists.txt
283

The code in AddFlang.cmake seems to build object libraries for calls to add_flang_library by default when STATIC argument is passed.

phosek added a subscriber: phosek.Jul 10 2023, 10:59 AM
phosek added inline comments.
flang-rt/CMakeLists.txt
18–24

I don't think we should support building FlangRT as a standalone project. We've been moving all runtimes to the runtimes build, the only leftover is compiler-rt and our hope is to address that soon.

42–52

None of this should be needed if building in the runtimes build.

pscoro updated this revision to Diff 538749.Jul 10 2023, 11:26 AM

Comment out some code that irrelevant to current object library approach

pscoro updated this revision to Diff 539113.Jul 11 2023, 8:09 AM

Some small changes, still non functional

pscoro updated this revision to Diff 539545.Jul 12 2023, 7:02 AM

Small changes, flang-rt seems to work with the flang-new driver now.

pscoro updated this revision to Diff 540026.Jul 13 2023, 7:25 AM

Fix driver usage of flang-rt

pscoro updated this revision to Diff 540110.Jul 13 2023, 10:54 AM

Fix build failures

vzakhari added inline comments.
flang-rt/CMakeLists.txt
18–24

Hi @phosek, can you please explain what you mean by "We've been moving all runtimes to the runtimes build"? Can you please share references to the pipeline for the "runtimes build"?

pscoro added inline comments.Jul 13 2023, 12:26 PM
flang-rt/CMakeLists.txt
18–24

If i understood correctly, i think @phosek is talking about building the flang-rt target directly (cmake llvm-project/flang-rt) when he says standalone. Meanwhile a runtimes build is cmake llvm-project/runtimes -DLLVM_ENABLE_RUNTIMES=flang-rt. I chose not to change anything yet because I was focused on some other bugs and was waiting for see if anybody else had input on this first. Also, in case my understanding is incorrect @phosek please correct me.

phosek added inline comments.Jul 13 2023, 11:22 PM
flang-rt/CMakeLists.txt
18–24

Yes, that is correct.

pscoro updated this revision to Diff 540413.Jul 14 2023, 7:27 AM

Decided to remove all the commented code related to the previous approach (wrapper library). The code that exists currently is for the object library approach and is functional.

Currently, on phabricator there is a failing x64 Windows build. The failure happens during building, the compiler seems to run out of heap space? I don't see how this error could be related to my patch but if I am wrong and it is, please let me know.

awarzynski added inline comments.Jul 16 2023, 10:08 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
982–992

Am I correct thinking that:

  • "<driver-path>/../lib" is for Fortran_main.a, and
  • "<driver-path>/../runtimes/runtimes-bins/lib " is for libflang-rt?

Could you document this? Thanks!

pscoro updated this revision to Diff 541040.Jul 17 2023, 8:05 AM

Added documentation for Flang-rt.

pscoro marked an inline comment as done.Jul 17 2023, 8:09 AM
pscoro added inline comments.Jul 17 2023, 8:09 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
982–992

Yep thats right, documentation added

awarzynski added inline comments.Jul 17 2023, 11:56 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
982–992

Yep thats right, documentation added

Could you document _where_ these libraries are located? Also, could you add some relevant comments here? For example:

// Add search path for XYZ
llvm::sys::path::append(BuildLibPath, "lib");
flang/docs/Flang-rt.md
15–16 ↗(On Diff #541040)

This statement is not quite true - the driver can do a lot without requiring these libraries. I suspect that you wanted o say that generating executables requires these libraries.

[nit] flang dirver --> Flang driver

17 ↗(On Diff #541040)

Motivation for ...? It's not clear what the following section aims to achieve.

I think that it would be good to take a step beck and decide who this documented is intended for. If it's the end-user then, imho, this section doesn't belong here. In documentation like this I'd focus on "what" and "how" as opposed to "why".

Even if this document is intended for developers, I wouldn't focus too much on the original motivations for this change and instead explain the design and its benefits:

  • possibility to build the runtime independently of Flang (what's the benefit of that?)
  • CMake set-up that's consistent with other sub-projects in LLVM,
  • a convenient wrapper for multiple (i.e. 2) Flang runtime libraries,
  • automated logic to build shared and static version of the runtime libs (to enable supporting e.g. -static?).

I hope that this makes sense.

30–31 ↗(On Diff #541040)

Could you document "why"?

35 ↗(On Diff #541040)

Perhaps turn this into a link to the CMake definition?

61–63 ↗(On Diff #541040)
  1. Please add an example.
  2. "you must add its path to the environment variable LD_LIBRARY_PATH" <-- this is just one way to provide a search path for the dynamic linker. There are other ways too. It would be good to clarify that a) one has to make the dynamic linker aware _where_ to look and that b) LD_LIBRARY_PATH is one such mechanism that can be used. But I would be careful not to "endorse" it - mishandling LD_LIBRARY_PATH can lead to tricky surprises.
pscoro updated this revision to Diff 541537.Jul 18 2023, 8:00 AM
pscoro marked an inline comment as done.

Updated documentation to address all nits

pscoro marked 6 inline comments as done.Jul 18 2023, 8:07 AM
pscoro added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
982–992

Could you document _where_ these libraries are located?

I've made the information more explicit in the documentation and added clarification that the ../runtimes/runtimes-bins/lib path happens for runtimes built as part of an llvm target build, but during standalone runtimes builds ../lib is used. (I am not sure why this behavior is what it is, but I don't think we should by changing or overriding the existing infrastructure)

pscoro updated this revision to Diff 541544.Jul 18 2023, 8:14 AM
pscoro marked an inline comment as done.

Removed support for "fully standalone" Flang-rt builds (ie CMake builds targeting llvm-project/flang-rt).
Flang-rt is now buildable via the llvm target or the runtimes target. See documentation for more
information.

pscoro marked 3 inline comments as done.Jul 18 2023, 8:19 AM
pscoro added inline comments.
flang-rt/CMakeLists.txt
18–24

I gave this a couple days in case anybody else in the community wanted to voice their opinion but since there was no contradictions, I have removed support for targeting flang-rt directly. Flang-rt is now only buildable via the llvm target or the runtimes target. See documentation for details.

42–52

I did some further testing and removed the module paths and include statements that were not needed. However, not all of the lines could be removed, clang and flang specific paths/includes were missing so they were left in here.

pscoro edited the summary of this revision. (Show Details)Jul 18 2023, 8:24 AM
pscoro marked 2 inline comments as done.
pscoro edited the summary of this revision. (Show Details)Jul 18 2023, 8:26 AM
vzakhari added inline comments.Jul 18 2023, 9:06 AM
flang-rt/CMakeLists.txt
18–24

Thank you for the explanation!

efriedma added inline comments.Jul 18 2023, 10:42 AM
flang/lib/Decimal/CMakeLists.txt
54

I think it would make sense to use explicit CMake variables for the "if' statements (i.e. make flang/CMakeLists.txt define BUILDING_FLANG, and make flang-rt/CMakeLists define BUILDING_FLANGRT) .

flang/runtime/CMakeLists.txt
282

This "if" doesn't make sense to me. If we're not building flang-rt, we shouldn't be here, so I don't see why you need an "if" in the first place.

pscoro added inline comments.Jul 18 2023, 12:09 PM
flang/runtime/CMakeLists.txt
282

add_subdirectory(runtime) is a line that still exists in flang/CMakeLists.txt. This exists because Fortran_main is still being built at the same time as the compiler, and to do so, the runtime subdirectory still needs to be added to flang (flang/CMakeLists.txt -> add_subdirectory(runtime) -> flang/runtime/CMakeLists.txt -> add_subdirectory(FortranMain). The solution I had was to just add a check around the FortranRuntime library production so that it only happens for flang-rt.

If you have a better solution let me know. Overall, I'm not sure if Fortran_main is currently being handled in the best way (ie, its still being built at the same time as the compiler, which doesn't seem ideal), but am not sure what course of action to take with it since it doesn't really belong in flang-rt either (see documentation for details)

efriedma added inline comments.Jul 18 2023, 12:24 PM
flang/runtime/CMakeLists.txt
282

Fortran_main should be "part of" flang-rt in the sense that building flang-rt builds it. Most of the same reasons we want to build flang-rt.a as a runtime apply.

Since the output needs to be separate, building flang-rt should produce two libraries: flang-rt.a and FortranMain.a.

pscoro added inline comments.Jul 19 2023, 7:55 AM
flang/runtime/CMakeLists.txt
282

I agree with this idea and have been trying to make it work but in the process I discovered that my "fix" above (if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES)) is worse than I thought.

By building the llvm target with flang-rt as an enabled runtime, we are essentially saying: build the flang compiler, and then invoke cmake again to build the runtimes project (externally), which builds flang-rt.

The problem is that check-all depends on check-flang which depends on the runtime. The if guard above was not actually doing what I thought it was, and the reason why configuring didnt fail with "flang-rt does not exist" is because the if guard was still evaluating to true. Basically, the guard ensured that FortranRuntime was only being built if flang-rt is built, but the add_library call was still being reached *during the configuration of the flang compiler*.

I am having trouble dealing with this dependency since runtimes get built externally (and after the compiler is built, which is when the check-all custom target gets built). I will have to investigate more closely how other runtimes handle this. My initial thought was perhaps the runtime testing should be decoupled from check-flang/check-all however I don't know if that's a good idea, and I also think that only helps with flang-rt, but if Fortran_main is required to build any executable I imagine that it should be needed for check-flang?

This is just an update of whats holding me back right now. If you have any tips please let me know. Thanks for bringing this to my attention.

efriedma added inline comments.Jul 19 2023, 1:13 PM
flang/runtime/CMakeLists.txt
282

For comparison, check-clang doesn't require any runtime libraries at all. All the checks only check the generated LLVM IR/driver commands/etc. Any tests that require runtime execution go into either the regression tests for the runtimes themselves, or into lllvm-test-suite.

Probably check-flang should do something similar. It probably makes sense to add a check-flang-rt target or something like that. (Not sure which existing flang tests are impacted.)

pscoro added inline comments.Jul 26 2023, 9:13 AM
flang/runtime/CMakeLists.txt
282

If we are decoupling the affected tests from flang-rt, then I think the only component left coupled (that I'd argue shouldn't be coupled) is the runtime source files. I'm seeing that for all the other runtimes in the llvm-project, the sources exist in a top level directory. Would it make sense and be feasible for flang-rt to act similarly?

Comparing to the infrastructure for compiler-rt, I've been entertaining the idea that FortranRuntime and FortranMain can be treated as a library with sources defined in flang-rt/lib/FortranRuntime and flang-rt/lib/FortranMain, similar to how compiler-rt has libraries like compiler-rt/lib/asan that are added to the final compiler-rt target.

FortranDecimalRT's sources would still be gathered from the Flang source directory, which would have to be known by flang-rt (also I see that multiple runtime files include flang/Common headers (but no sources get linked), so flang-rt would need to be aware of flang's directory for that reason too)

Do you think this idea worth pursuing or has the scope expanded way past the original intention?

klausler added inline comments.Jul 26 2023, 9:19 AM
flang/runtime/CMakeLists.txt
282

This would make life more difficult for people (okay, me) doing most of their flang development in an out-of-tree mode.

efriedma added inline comments.Jul 26 2023, 9:39 AM
flang/runtime/CMakeLists.txt
282

I think reorganizing the source-code makes sense... but I suspect it'll turn into its own giant discussion, and it'll make it harder to review the patches. So maybe avoid doing that for now if you can.

pscoro updated this revision to Diff 550389.EditedAug 15 2023, 10:37 AM

Added testing support and reworked a lot of the existing patch. check-flang-rt now exists and flang-rt is more fully structured and finished. This update includes up-to-date documentation on building/usage.

I will update the community slack and the discourse thread to make everyone aware that this patch is ready to be revisited

pscoro added inline comments.Aug 15 2023, 10:47 AM
flang/runtime/CMakeLists.txt
282

It took a couple weeks but the support for testing in flang-rt now exists. I understand that the size of the patch has grown significantly as a result. I will work with the community closely to try to make this digestible and to address and comments or concerns.

flang/runtime/sum.cpp
141

@klausler I don't plan on including this in this patch. This is related to https://reviews.llvm.org/D127025. I get an error about line 51:

auto next{x + correction_};

when building flang-rt via the runtimes target. It complains because x is a __float128 and correction is a long double. I am unsure as to why as I am unfamiliar with what this code does. Looking at the patch linked above I saw code similar to my addition here in other files so I tried to add it here and it seemed to fix my issues. It is just a wild guess though. Please let me know if you have anymore information or intuitions regarding this bug.

pscoro edited the summary of this revision. (Show Details)Aug 15 2023, 10:50 AM
pscoro added reviewers: DanielCChen, kkwli0, madanial.

Some of the testcases have been reformatted by clang-format when I updated the patch with arcanist. If the community is against introducing extra changes like these I would be happy to re-update my patch without clang-formatting these files.

In order to keep the scope of this patch manageable, and because further discussion is warranted first, no source files have been moved from flang into flang-rt. Although I would imagine that this may be something we would want to do in the future in order to continue our efforts at decoupling compiler and runtime.

The test changes look good to me.

flang/lib/Decimal/CMakeLists.txt
52–67

This add_compile_options(-fPIC) shouldn't be necessary?

pscoro added inline comments.Aug 15 2023, 11:18 AM
flang/lib/Decimal/CMakeLists.txt
52–67

Pretty sure you're right, let me build/test without it to make sure and I'll update if everything is alright

pscoro updated this revision to Diff 550422.Aug 15 2023, 12:08 PM

Removed a few unnecessary lines of cmake

pscoro marked 7 inline comments as done.Aug 15 2023, 12:11 PM
pscoro marked an inline comment as done.Aug 16 2023, 8:24 AM
pscoro retitled this revision from [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration to [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration.Aug 16 2023, 8:34 AM
pscoro edited the summary of this revision. (Show Details)
pscoro updated this revision to Diff 550809.Aug 16 2023, 10:55 AM

Updated docs on using runtimes target, specified that -DCMAKE_C_COMPILER=$BUILD_DIR/bin/clang should be set. It seems that a small set (4) of gtest unittests do not run if the default /usr/bin/cc compiler is used.

pscoro updated this revision to Diff 551125.Aug 17 2023, 7:19 AM

Reverting change to docs, irrelevant.

pscoro marked an inline comment as not done.Aug 17 2023, 7:22 AM

Ping

I will not be able to help out with flang (for a while) after the end of August so the sooner I can get this reviewed the smoother the landing process will be for myself and the community. So I would appreciate if I could get reviewers looking at this as soon as possible. Thanks

klausler added inline comments.Aug 18 2023, 7:44 AM
flang-rt/unittests/FortranEvaluate/testing.h
2

Why was this file (and testing.cpp) added instead of being moved, like other files were?

Why are the unit tests for the Evaluate library being moved into flang-rt? The Evaluate library is only used by the compiler for constant folding and expression processing; it is not part of the runtime.

pscoro added inline comments.Aug 18 2023, 7:57 AM
flang-rt/unittests/FortranEvaluate/testing.h
2

There are 2 unit tests (reshape and ISO-Fortran-Binding) that include runtime headers. Neither of these tests included headers from any other Flang libraries so I chose to move them under flang-rt. I preserved the testsuite name FortranEvaluate because thats where they came from. I am not too confident in this decision. Let me know if there is an alternative you prefer.

These testing files were copied because they are needed for other tests that remained in flang/unittests/FortranEvaluate and for these 2 unit tests that I moved to flang-rt

pscoro updated this revision to Diff 551799.Aug 19 2023, 6:32 PM

Removed a todo

Pinging for review. I will be taking over this patch from Paul, but Paul will still be around to answer any questions.

Maybe split the changes to reformat the unittests into a separate patch?

Otherwise, I'm happy with the current state of this patch, but probably someone more active in flang should approve.

ldionne added a subscriber: ldionne.Sep 8 2023, 6:14 AM
ldionne added inline comments.
runtimes/CMakeLists.txt
20

This shouldn't be necessary, CMake already creates a Project_SOURCE_DIR variable for each declared project() IIRC.

Any update on this patchset? With the migration away from phabricator, it'd be good to get this in soonish (otherwise it'll need to be moved to github).

madanial updated this revision to Diff 557468.Sep 28 2023, 2:10 PM

Using the Project_SOURCE_DIR variable in cmake instead of setting a new variable

runtimes/CMakeLists.txt
20

Updated thanks for the review

Maybe split the changes to reformat the unittests into a separate patch?

Otherwise, I'm happy with the current state of this patch, but probably someone more active in flang should approve.

I believe the changes are very trival to the unittests, would we be able to keep them as part of this patch and avoid moving it to github

DanielCChen accepted this revision.Sep 29 2023, 8:46 AM

@all
It seems all the comments are addressed or explained. If there is no objection from the reviewers, we would like to land this patch in Phabricator before the deadline.
I will approve it now, but @madanial please wait until the EOD to see if other reviewers have further comments.

We will address any further suggestion of change as separate PRs.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 30 2023, 9:54 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

With this patch, it started to fail with:
It would be nice to have a better error message suggesting what to do

-- Performing Test HAVE_DECL_STRERROR_S
-- Performing Test HAVE_DECL_STRERROR_S - Failed
CMake Error at /build/source/flang-rt/unittests/CMakeLists.txt:37 (message):
  Target llvm_gtest not found.


-- Configuring incomplete, errors occurred!
pscoro added a comment.Oct 1 2023, 9:59 AM

With this patch, it started to fail with:
It would be nice to have a better error message suggesting what to do

-- Performing Test HAVE_DECL_STRERROR_S
-- Performing Test HAVE_DECL_STRERROR_S - Failed
CMake Error at /build/source/flang-rt/unittests/CMakeLists.txt:37 (message):
  Target llvm_gtest not found.


-- Configuring incomplete, errors occurred!

Hi, I originally authored this patch but I am no longer actively involved in Flang's development at the moment, however, I've been getting notifications that this patch has been landed yesterday and subsequently, the failing buildbots.
I believe all the gtest not found errors originate from a missing build flag, as per the documentation page for flang-rt in flang-rt/docs/GettingStarted.md:

# We need to enable GTest if we want to run Flang-rt's testsuites
-DLLVM_INSTALL_GTEST=On \

I believe I saw that clang buildbots also got inadvertently by this cmake change as well.
Wish I had time to investigate this further myself, hopefully @madanial can take a closer look. I believe what needs to be done is:

  • flang buildbots need their build flags updated to include -DLLVM_INSTALL_GTEST=On
  • The Target llvm_gtest not found. should be improved to offer better guidance
  • The clang failures should be looked at closer; see if its possible to make a change to the cmake to limit the scope of builds needing the -DLLVM_INSTALL_GTEST=On flag update to just be flang builds. If there is no simple way to limit the scope, then updating the build flags for the clang builders is another solution
  • I noticed at least one flang builder (https://lab.llvm.org/buildbot/#/builders/191/builds/24124) is failing build with:
FAILED: bin/external-hello-world 
: && /usr/local/bin/c++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-copy -Wno-string-conversion -Wno-ctad-maybe-unsupported -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3 -DNDEBUG -Wl,-rpath-link,/home/tcwg-buildbot/worker/flang-aarch64-rel-assert/build/./lib  -Wl,--gc-sections tools/flang/examples/ExternalHelloWorld/CMakeFiles/external-hello-world.dir/external-hello.cpp.o -o bin/external-hello-world  -Wl,-rpath,"\$ORIGIN/../lib:"  -lpthread  -lFortranRuntime && :
/usr/bin/ld: cannot find -lFortranRuntime

This looks to be a usage of the old FortranRuntime configuration that I overlooked and so it wasnt updated. This should be linking flang-rt instead of FortranRuntime

Hopefully addressing these small issues will get the buildbots green again, sorry for the disruptions.

I've reverted this https://github.com/llvm/llvm-project/commit/ffc67bb3602a6a9a4f886af362e1f2d7c9821570 as Linaro's flang and clang bots are red all over and we're also in an upgrade period for the build server (https://discourse.llvm.org/t/llvm-zorg-migration-to-the-buildbot-v3-9/73749) so getting a fix into zorg isn't going to happen for a few days at least.

Whoever decided to land this change please look at the remedies suggested by @pscoro and submit PRs to address those and eventually reland this. Happy to help on review.

Thanks for the revert!

DanielCChen added a comment.EditedOct 3 2023, 9:36 AM

Sorry about the breakage and thanks to @DavidSpickett for reverting it.
We will reconsider all the input and bring it back as a PR in the future.

Sorry about the breakage

This is why we have CI ;)