Skip to content

[purs ide] Updates the cache-db.json file on rebuilds #3789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Feb 24, 2020

Conversation

kritzcreek
Copy link
Member

This patch makes it so we update the content hashes when rebuilds through the IDE occur. We've still got some unfortunate invalidation going on when recompiling a file with no changes, but at least this way the IDE can never cause purs compile to invalidate too little.

One non-local change is that we now do some path normalisation for the cache-db entries, so two invocations like purs compile "src/*.purs" and purs compile "./src/*.purs" don't cause the compiler to invalidate every module.

I plan to do the file watcher replacement in a follow-up PR.

@kritzcreek kritzcreek requested a review from hdgarrood February 13, 2020 16:03
@kritzcreek kritzcreek changed the title [purs ide] Updates the cache-db.json file when trigger rebuilds [purs ide] Updates the cache-db.json file on rebuilds Feb 13, 2020
@hdgarrood
Copy link
Contributor

I just want to check again that I've understood the issues you're solving here properly, does the following sound correct?

  • firstly, we don't normalise file paths in the cache db, which means that we end up with IDE rebuilds invalidating everything below the rebuilt module even if there are no changes, because e.g. "./path/to/module.purs" is considered different to "path/to/module.purs", and if you're building things both with purs compile directly and with the ide you can easily end up with a mixture
  • secondly, we don't update the cache-db.json file at all when the ide rebuilds a particular module, which means that the source file's timestamp has changed on disk but the cache db still contains the old timestamp, which causes the cache to be invalidated for that file, causing it and everything below it to be rebuilt on the next full build, even when there are no changes

Is that it, or is there more to it than this?

@kritzcreek
Copy link
Member Author

firstly, we don't normalise file paths in the cache db, which means that we end up with IDE rebuilds invalidating everything below the rebuilt module even if there are no changes, because e.g. "./path/to/module.purs" is considered different to "path/to/module.purs", and if you're building things both with purs compile directly and with the ide you can easily end up with a mixture

Yes, in addition to that mixing absolute with relative filepaths has the same effect.

secondly, we don't update the cache-db.json file at all when the ide rebuilds a particular module, which means that the source file's timestamp has changed on disk but the cache db still contains the old timestamp, which causes the cache to be invalidated for that file, causing it and everything below it to be rebuilt on the next full build, even when there are no changes

That's the harmless case. With the current situation you can imagine the following sequence:

  1. The user makes an edit without saving
  2. The IDE rebuilds against a temp file and writes out a changed externs file
  3. The user discards the unchanged save without triggering a rebuild
  4. The output/ directory now contains an externs file that does not correspond to the .purs file on disc, but can't be invalidated by a normal purs compile because the cache says the timestamps and content hashes match

@hdgarrood
Copy link
Contributor

Ah yes, thanks for reminding me.

Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@kritzcreek
Copy link
Member Author

Thanks for taking a look!

@kritzcreek kritzcreek merged commit fcc7a08 into purescript:master Feb 24, 2020
@kritzcreek kritzcreek deleted the ide-cache-db branch February 24, 2020 01:16
justinwoo pushed a commit to justinwoo/purescript that referenced this pull request May 15, 2020
* generalizes and extracts CacheDb accessors from Make

This is so they can be used from within the IDE as well, which doesn't
run in Make

* overwrites ContentHashes and Timestamps for rebuilt modules

* removes a whole lot of "Christoph didn't know what he was doing"

* reorganizes the cache info building

* normalises filepaths before inserting them into the Cache

* normalise file paths when rebuilding from the IDE

* extracts the logic that updates the Cache

* inlines function that I didn't up using in the IDE code

* cleaner diff

* more simplifications

* Update src/Language/PureScript/Make/Cache.hs
@hdgarrood hdgarrood mentioned this pull request May 23, 2020
hdgarrood pushed a commit that referenced this pull request May 23, 2020
* generalizes and extracts CacheDb accessors from Make

This is so they can be used from within the IDE as well, which doesn't
run in Make

* overwrites ContentHashes and Timestamps for rebuilt modules

* removes a whole lot of "Christoph didn't know what he was doing"

* reorganizes the cache info building

* normalises filepaths before inserting them into the Cache

* normalise file paths when rebuilding from the IDE

* extracts the logic that updates the Cache

* inlines function that I didn't up using in the IDE code

* cleaner diff

* more simplifications

* Update src/Language/PureScript/Make/Cache.hs
@wclr
Copy link
Contributor

wclr commented Aug 25, 2021

@kritzcreek

  • The user makes an edit without saving
  • The IDE rebuilds against a temp file and writes out a changed externs file

Why should IDE write out a changed a externs file if it doesn't produce the codegen output (codegeTargets is empty)? It seems that if no output is produced and externs/cache-db is not touched this couldn't break anything for later purs compile. And this would allow having absolutely pure diagnostics for non-saved files in the editors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants