Fix recompilation checking of pure plugins
ClosedPublic

Authored by DanielG on Nov 4 2018, 5:52 PM.

Details

Summary

Previously when switching from using a Plugin with
RecompMaybe/ForceRecompile in pluginRecompile to a Plugin with
NoForceRecompile GHC would never even consider recompiling.

However the previously active plugin could have modified the
compilation output so we should recompile.

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.Nov 4 2018, 5:52 PM
DanielG added inline comments.Nov 4 2018, 6:01 PM
compiler/main/DynFlags.hs
2594

Since I'm mostly interested in the behaviour of plugins in the GHC API / GHCi I had to add a way to reset the currently active plugins. Lower case names aren't valid haskell module names, right? So using a lower case word as a keyword here should be fine.

If you guys thing this is OK I can add some docs for this option.

testsuite/tests/plugins/T15858.script
9

I sure hope the GHC tests have echo in their PATH on werid platforms like WinDOS :)

testsuite/tests/plugins/all.T
205

All the other plugin tests seem to have this predicate but if I enable it the test doesn't actually run if I do make TEST=T15858. Anybody have any idea what's going on there?

I'm quite confused; I would have thought that the change in the module's Usages would have been sufficient to trigger a recompilation. @mpickering, do you know what is going on here?

mpickering requested changes to this revision.Nov 8 2018, 4:02 PM

Looks plausible but I need to understand what Ben meant in his comment above before understanding the logic in detail.

Also the change to addPluginModuleName is definitely not desirable, please change that.

compiler/main/DynFlags.hs
2594

No, please don't do this. Add a new function called clearPlugins which does it.

testsuite/tests/plugins/T15858.script
2

Aren't the other plugin tests specified in a makefile rather than as a ghci script? Why did you choose this?

31

typo

This revision now requires changes to proceed.Nov 8 2018, 4:02 PM
DanielG added a comment.EditedNov 8 2018, 4:34 PM
This comment has been deleted.
compiler/main/DynFlags.hs
2594

This is just a quick hack, I was going to rename the function later actually.

I'm more talking about whether it's ok to reuse the -fplugin option with a =clear argument (to keep it consistent with -package-db) instead of adding a new option.

testsuite/tests/plugins/T15858.script
2

It's just the specific case I'm interested in. See my StaticPlugin patch.

I started writing this test when I still didn't know if this was GHCi specific or not and it turns out GHCi has an additional caching layer that never recompiles when plugin fingerprints change (but that's a seperate bug report) unless you use -fobject-code.

I can port this to regular GHC in the makefile if you like, this was just easier for me to produce since I was already working on testing out plugin recomp in GHCi.

DanielG updated this revision to Diff 18878.Sun, Nov 25, 9:49 AM

Add -fclear-plugins (incl. docs) instead of overloading the -fplugin option.

DanielG updated this revision to Diff 18880.Sun, Nov 25, 9:53 AM
DanielG marked 3 inline comments as done.

Fix typo

compiler/main/DynFlags.hs
2594

Actually I was thinking of cabal-install's --package-db option that has this =clear argument behaviour. GHC has seperate options for that elsewhere so I'm doing that now instead.

DanielG marked 2 inline comments as done.Sun, Nov 25, 9:54 AM
bgamari accepted this revision.Tue, Dec 11, 5:36 PM
bgamari added inline comments.
testsuite/tests/plugins/all.T
205

I will take care of this.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Dec 12, 10:27 PM
This revision was automatically updated to reflect the committed changes.