Remove invalid entries from cache-db #4407
Conversation
Modules are now removed if they are not being compiled and another module has the same source.
EMattfolk
changed the title
|
Could this be merged? |
|
@i-am-the-slime as documented here merging things to the compiler requires two people from core to approve this. I would have a look at this myself but I forgot how this chunk of the compiler works and I'm extremely swamped at the moment. Given that it's the Thanksgiving week a lot of folks might be offline, so it will probably be at least one more week before anyone can look at it. |
src/Language/PureScript/Make.hs
Outdated
| @@ -136,7 +136,7 @@ rebuildModuleWithIndex MakeActions{..} exEnv externs m@(Module _ _ moduleName _ | |||
| -- | |||
| -- If timestamps or hashes have not changed, existing externs files can be used to provide upstream modules' types without | |||
| -- having to typecheck those modules again. | |||
| make :: forall m. (Monad m, MonadBaseControl IO m, MonadError MultipleErrors m, MonadWriter MultipleErrors m) | |||
| make :: forall m. (Monad m, MonadBaseControl IO m, MonadError MultipleErrors m, MonadWriter MultipleErrors m, MonadIO m) | |||
There was a problem hiding this comment.
What is this MonadIO constraint added for?
There was a problem hiding this comment.
I needed to check which files existed in order to remove them from the cache which meant I had to do IO. I'm kind of new to haskell but my LSP suggested I add that constraint and it just worked. It felt kind of like it shouldn't be there when I wrote it, not sure how I would do it otherwise
There was a problem hiding this comment.
Right, I just couldn't see what required this constraint straight away
src/Language/PureScript/Make.hs
Outdated
| (\acc1 e -> case e of | ||
| -- This pattern will clean up duplicates over time. | ||
| -- It will keep the cache in a good state if it was in a good state before. | ||
| m1 : m2 : _ -> |
There was a problem hiding this comment.
isn't this just when Set.size e > 1 then add e \\ modules to the accumulator? Then there's no need to wait until multiple compilations reach a fixed point (assuming I understand the comment)
There was a problem hiding this comment.
I don't think so? What this means is if there are two or more modules with the same name in the cache, at most one of them is being compiled right now (i.e. it is in the modules set). e \\ modules (assuming \\ is set difference) will be all duplicate modules, but we want to ignore those, not find them. Perhaps e \\ (e \\ modules) is easier to understand?
Anyway, in practice it is possible there are multiple duplicates if you rename and compile several times, but after this change it is only possible to have a single duplicate, so this code is enough. If you think it is better to refactor it I will do that
There was a problem hiding this comment.
Sorry, not sure if I was unclear or I don't understand - I was suggesting that e \\ modules is added to the accumulator that goes into this fold to construct toRemove, as acc2 = S.union (Set.singleton m1 \\ modules) acc1 etc
I don't feel strongly about the code, just trying to follow through and understand "clean up duplicates over time"
There was a problem hiding this comment.
I think I understand what you mean now. I must have been tired when I read your comment initially :) Yes, just doing the set difference is probably the way to go. I will refactor it
There was a problem hiding this comment.
While you're in there, I think this might be a bit clearer if you separate out the duplicate pruning and the missing file pruning into separate functions.
Also, general tip: you're doing a lot of bespoke folds here to accumulate and aggregate stuff, and I think you'd be better off using simpler primitives like concat and filter and M.fromListWith.
|
I may have accidentally simplified the implementation quite a bit. I believe the behavior is the same. What do you think about also removing the old compiled output while we're at it? @rhendric @nwolverson Or should this be merged as-is? Edit: looks like something broke. I think I remember discussing about a usecase where the cache remembers modules that were switched out or similar. Not sure I agree that should be a feature, but I it what it is I guess |
My general feeling was this PR is currently uncontroversial and at this point probably want to get it merged rather than expanding scope. Apologies for not noticing the question and responding sooner. I don't know what the build problem was - can we kick that (perhaps via bringing up to date with latest) and see where we stand? |
Makes sense, this PR wasn't supposed to do that from the beginning anyway.
Do you mean fixing the code which is failing the test cases that I wrote? Or to relax the tests a bit to allow the current version through? I'm more in favor of the second as that will simplify the implementation (as well as giving more predictable behaviour). Either way I could probably get a fix in quite quickly since I have more time for these kinds of things nowadays 🙂 |
|
I mean there are no logs for the failed build checks (from what I see) so it's not clear if they were merely "build issues" or actual concrete things to be resolved. I assume from your comment that there was a test failure - I'm happy to comment on a particular case if you can give a pointer |
Description of the change
This is a step in the process of resolving #4133.
cache-db.jsoncurrently contains references to files that sometimes does not exist which, while not harmful, is a bit confusing.Fully fixing #4133 would also require deleting the compiled modules for which the source files have been deleted. I do not know the implications of deleting things from the
outputfolder which is why I skipped that for now. Adding it in later should be fairly trivial if that is the route to take. I have had some discussions about this in the past and it seems to be a rather controversial topic. I have also seen many problems arise because old output is left around, so I would like to get some more discussion going.Note
This change successfully removes modules from
cache-db.jsonif the source file is removed or the module is renamed, so it works fine when:It does not work when only the module name is changed inside the file.Fixed. Though I think this could be detected by doing the following:Checklist: