Support registering Plugins through the GHC API
ClosedPublic

Authored by DanielG on Oct 28 2018, 9:31 PM.

Details

Summary

This allows tooling using the GHC API to use plugins internally.
Hopefully this will make it possible to decouple the development of
useful plugins from (currently) kitchen-sink type tooling projects
such as ghc-mod or HIE -- at least to some extent.

Test Plan

validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
DanielG created this revision.Oct 28 2018, 9:31 PM
DanielG updated this revision to Diff 18517.Oct 28 2018, 9:42 PM

Rebase onto master

Something werid is going on with the CI build, it immediately fails with:

exception 'PhabricatorWorkerPermanentFailureException' with message 'Lease "PHID-DRYL-sgmvjcrb3e4vdnju3r72" never activated.' in /opt/phabricator/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php:91
Stack trace:
#0 /opt/phabricator/src/applications/harbormaster/worker/HarbormasterTargetWorker.php(70): HarbormasterLeaseWorkingCopyBuildStepImplementation->execute(Object(HarbormasterBuild), Object(HarbormasterBuildTarget))
#1 /opt/phabricator/src/infrastructure/daemon/workers/PhabricatorWorker.php(123): HarbormasterTargetWorker->doWork()
#2 /opt/phabricator/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php(171): PhabricatorWorker->executeTask()
#3 /opt/phabricator/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php(22): PhabricatorWorkerActiveTask->executeTask()
#4 /opt/libphutil/src/daemon/PhutilDaemon.php(219): PhabricatorTaskmasterDaemon->run()
#5 /opt/libphutil/scripts/daemon/exec/exec_daemon.php(131): PhutilDaemon->execute()
#6 {main}

It's not clear to me why we need a separate type for API plugins. Why does this facility appear to be so different from the "usual" plugin mechanism?

alanz added a subscriber: alanz.Oct 29 2018, 2:03 AM
mpickering requested changes to this revision.Oct 29 2018, 4:54 AM
mpickering added a subscriber: mpickering.

What is the problem with adding the plugin you want to load to the plugins field? There at least needs to be a comment explaining why it is necessary for these two different approaches which look very similar.

This revision now requires changes to proceed.Oct 29 2018, 4:54 AM
DanielG added a comment.EditedOct 29 2018, 12:43 PM

FYI there is more commentary in the linked trac issue. I wasn't sure whether I should also copy that into the commit description or not, is that desirable?

Essentially there's two problems with just using the old plugins field (as I explain in the trac issue): (1) it is treated as a cache and overwritten whenever the code sees fit. (2) how do you come up with a ModIface for a module that's not even being compiled right now. So at the very least lpModule would have to become optional.

I chose to introduce a new type and rename the old field because that allowed me to let GHC direct me to the places that need fixing.

DanielG updated this revision to Diff 18544.EditedOct 31 2018, 8:14 PM

Rename to "StaticPlugin" to clarify use-case, add some more doc comments and update commit message.

bgamari added inline comments.Nov 2 2018, 1:03 PM
compiler/main/Plugins.hs
154

I really don't like this design of mirroring LoadedPlugin. LoadedPlugin and StaticPlugin are so similar; the only difference is that one lacks a ModIface. Why not just make the lpModule a Maybe?

Also, we still do need a test to verify that recompilation works properly.

DanielG updated this revision to Diff 18588.Nov 4 2018, 4:44 PM

Hook-up recompilation logic for StaticPlugin and add tests for recompilation

DanielG marked an inline comment as done.Nov 4 2018, 4:48 PM
DanielG added inline comments.
compiler/main/Plugins.hs
154

Yup, valid point. I've chosen to pull the common bits into PluginWithArgs instead of following your suggestion since I prefer to keep the concepts of dynamically loaded LoadedPlugin and statically loaded StaticPlugin as seperate types.

DanielG marked an inline comment as done.Nov 9 2018, 6:04 AM
DanielG updated this revision to Diff 18874.Sun, Nov 25, 7:45 AM

Rebase on master

DanielG updated this revision to Diff 18876.Sun, Nov 25, 7:54 AM

Fix arc diff fuckup (hopefully)

bgamari requested changes to this revision.Tue, Dec 11, 12:24 PM

Alright, looks good. Let's merge this.

This revision now requires changes to proceed.Tue, Dec 11, 12:24 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Tue, Dec 11, 5:22 PM
This revision was automatically updated to reflect the committed changes.