Cache last rebuild by kritzcreek · Pull Request #2083 · purescript/purescript

kritzcreek · 2016-05-02T09:08:08Z

Rebased on top of the 0.9 branch.

I think this is ready for review. If you can think of a way to make reviewing changes like this easier let me know. I can offer to implement the functionality in my psc-ide-emacs fork but I don't think anyone but me is using emacs?

hdgarrood · 2016-05-02T13:20:01Z

I'm actually giving spacemacs another try at the moment.

LiamGoodacre · 2016-05-02T13:47:10Z

I use spacemacs too.

kritzcreek · 2016-05-02T18:40:34Z

@hdgarrood @LiamGoodacre well if you want to give it a try you can stack install the binaries from this branch and use my private layer. Using a private layer means copying it into the .emacs/private directory,

I'll document it 🔜, but for now if you swap the purescript layer for the kc-purescript layer you can open a file in a purescript project and do:

, m s Return - Start the server
, m l - Load all modules

If you now try to get completions in a module you should only be able to complete exported identifiers.

, m b - rebuild

If rebuilding succeeds and you try to get completions you should now also get suggestions for private identifiers.

P.S. rebased again

paf31 · 2016-05-02T20:14:56Z

Do we know why the build fails?

kritzcreek · 2016-05-02T20:20:32Z

Sorry, but I have no idea O.o

paf31 · 2016-05-02T20:21:17Z

Looks like Appveyor is having trouble with Node.

kritzcreek · 2016-05-02T20:32:49Z

@garyb Could you maybe run the tests for this branch on windows, that would be really nice? I don't see why this wouldn't work on windows.

garyb · 2016-05-02T20:45:21Z

I can do sure, I think this is just an AppVeyor glitch though. @hdgarrood can you restart it? Also, is there any way you can give us permission to do stuff with setup/restarting on there too?

hdgarrood · 2016-05-02T20:55:39Z

I've restarted it. I have no idea how to give you access, though. :(

garyb · 2016-05-02T22:34:13Z

Hmm, it timed out again, weird. Got a lot further that time though.

@hdgarrood No problem about the AppVeyor thing, you'd think it would just work anyway, as it should be able to tell from our organisation membership the way Travis does.

texastoland · 2016-05-02T22:38:08Z

Maybe http://help.appveyor.com/discussions/questions/1459-mirroring-github-organization-permissions? Y'all are doing awesome code but I figured I could at least google in between configuring WebPack 😄

garyb · 2016-05-02T22:45:03Z

That probably deserved a "lmgtfy", I was just too lazy to check 😆. Thanks though! I need to do the same for SlamData soon.

hdgarrood · 2016-05-02T23:56:50Z

Thanks @AppShipIt, I think that should do the trick. Anyone in the owners team of the purescript org should be able to restart builds now, I think.

texastoland · 2016-05-02T23:57:57Z

Much obliged jealous of y'all's productivity today 👏

kritzcreek · 2016-05-04T08:55:43Z

Sorry but I have no idea why this fails on appveyor. I can squash and have it rebuild again but it's just guessing from my side.

garyb · 2016-05-04T10:25:12Z

I'll try it locally, see if there's any more info.

garyb · 2016-05-04T11:03:14Z

Nope, no more useful info, but it does get up to:

Language.PureScript.Ide.Imports.Integration
  Adding imports

and then hangs indefinitely... well, for at least 5 minutes before I killed it anyway, and more like 60mins on AppVeyor by the looks of it, since it's timing out there. 😢

kritzcreek · 2016-05-04T11:05:24Z

Argh... Thank you for the information! I'll see if I can add in a few timeouts and see where it hangs.

garyb · 2016-05-04T11:07:13Z

The only other thing I can say is it doesn't appear to be using any CPU time and the memory usage is static when it gets stuck too.

kritzcreek · 2016-05-06T16:20:46Z

Setting up a Windows environment atm. I have no idea what is going on :D

garyb · 2016-05-06T16:46:06Z

Doh 🙈

kritzcreek · 2016-05-06T17:52:16Z

Sank enough time into this for today... this is really frustrating :D It looks like something is deadlocking, but I think that really shouldn't happen when I'm only using STM.

kritzcreek · 2016-05-06T21:59:03Z

tests/support/pscide/src/RebuildSpecWithHiddenIdent.purs


 hidden x _ = x
+
+exported :: forall a. a -> a


@paf31 @garyb This is the change that fixes the hanging. Without it psc gets stuck. I can't reproduce it on the commandline though.... it only happens in compile in tests/Language.PureScript.Ide.RebuildSpec, only on Windows.

kritzcreek · 2016-05-08T21:21:47Z

Squashed and rebased on the newest 0.9 branch

paf31 · 2016-05-14T19:16:14Z

Perhaps it is easier to just do externs generation twice then?

Edit: that does thave the downside that we need to change the Make API though.

kritzcreek · 2016-05-14T19:21:03Z

I kind of talked about this with @hdgarrood and @nwolverson and they didn't like the idea that this would now allow users to access private members of their modules. I was kind of asking for this in #1620.

Writing the opened externs file to disc that is.

paf31 · 2016-05-14T19:23:37Z

I tend to agree, but we don't have to save the modified externs.

coveralls · 2016-05-14T21:11:25Z

Coverage decreased (-0.3%) to 55.627% when pulling 18723c7 on kRITZCREEK:cache-last-rebuild into 46f1687 on purescript:0.9.

kritzcreek · 2016-05-15T11:49:10Z

rebased again and adopted Rebuild.hs to utilize #2126

kritzcreek · 2016-05-15T11:58:05Z

I don't think changing the Make API to accomodate for this very narrow usecase is a good idea. I still think it's a good idea to give tools the ability to turn off the cache, because tools that only report errors (pscid, webpack-loader) and don't offer any completions shouldn't have to wait twice as long.

I image that Atom or Emacs would turn it on by default and allow the user to disable it with an option.

Once we start working on emulating ghci's :load for psci we can still look to unify the two approaches but I think it's best to defer that decision until we know what we need for psci. It's not that much code after all.

EDIT: The CI failure is some git problem on OSX

coveralls · 2016-05-15T12:36:55Z

Coverage decreased (-0.3%) to 55.612% when pulling 8e61d9a on kRITZCREEK:cache-last-rebuild into 7f29fad on purescript:0.9.

paf31 · 2016-05-15T19:04:14Z

What if the secondary build were sparked on another thread in the background? That way, we can cache the result, but not hold up the UI. You'd have to be careful to avoid race conditions when updating the state of course.

kritzcreek · 2016-05-15T23:25:38Z

I don't feel confident handling concurrency yet. Spinning up worker threads makes testing a lot harder and I don't want to take that complexity until we have a really good reason.

Thinking about this a bit more, I am fine with just removing the flag and always rebuilding twice, since a lot of the "rebuild" time is actually spent sorting the externs and we don't have to do that twice.

coveralls · 2016-05-16T00:29:05Z

Coverage decreased (-0.3%) to 55.607% when pulling 9bfa4ff on kRITZCREEK:cache-last-rebuild into 2da4825 on purescript:0.9.

It's cached with its exports opened up, so that we can source completions from private members when we are editing a module. * Adds documentation to the functions inside ...Ide.State and breaks the functions into a pure and a stateful part to minimize time spent in STM * Cleans up json instances for Command to use Applicative style where possible

rebuildModule inside ...Make already does that now

Now that multiple modules per file are forbidden, we can simplify the parsing code in Rebuild a bit.

kritzcreek · 2016-05-16T10:52:49Z

Rebased and merged again :)

coveralls · 2016-05-16T11:58:26Z

Coverage decreased (-0.3%) to 55.661% when pulling 4396720 on kRITZCREEK:cache-last-rebuild into 4852b5d on purescript:0.9.

kritzcreek · 2016-05-17T11:05:59Z

Is this PR going in with 0.9? It's not a breaking change so if it's too much to review until then we could easily put it into 0.9.1

paf31 · 2016-05-17T15:36:45Z

I think it should, but I won't be able to review it for a couple of days.

Sent from my iPhone

On May 17, 2016, at 4:06 AM, Christoph Hegemann notifications@github.com wrote:

Is this PR going in with 0.9? It's not a breaking change so if it's too much to review until then we could easily put it into 0.9.1

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

paf31 · 2016-05-22T18:21:44Z

src/Language/PureScript/Ide/Rebuild.hs

+    Left parseError ->
+      throwError . RebuildError . toJSONErrors False P.Error $ parseError
+    Right [m] -> pure m
+    Right _ -> throwError . GeneralError $ "Please define exactly one module."


I think this case can be an internalError now, since we only allow a single module per file.

Seems like you're looking at an outdated diff. I did that in this PR

Oh, weird. Thanks, looks good.

paf31 · 2016-05-22T18:22:53Z

Looks good to me!

paf31 · 2016-05-22T18:23:05Z

Thanks @kritzcreek !

kritzcreek · 2016-05-22T18:24:27Z

Cool thanks!

* Cache the last rebuilt module inside psc-ide state It's cached with its exports opened up, so that we can source completions from private members when we are editing a module. * Adds documentation to the functions inside ...Ide.State and breaks the functions into a pure and a stateful part to minimize time spent in STM * Cleans up json instances for Command to use Applicative style where possible * Don't add a default import for Prim rebuildModule inside ...Make already does that now * only parse a single module when rebuilding Now that multiple modules per file are forbidden, we can simplify the parsing code in Rebuild a bit. * remove the cacheSuccess flag * fix docs

kritzcreek mentioned this pull request May 2, 2016

[psc-ide] Cache last rebuild #2058

Closed

paf31 added the status: ready for review label May 2, 2016

kritzcreek force-pushed the cache-last-rebuild branch from 6732395 to 3b3bc8b Compare May 2, 2016 18:50

kritzcreek force-pushed the cache-last-rebuild branch from 3b3bc8b to feb569d Compare May 5, 2016 18:55

kritzcreek force-pushed the cache-last-rebuild branch from d9e01b4 to ed5fa86 Compare May 6, 2016 21:39

kritzcreek reviewed May 6, 2016
View reviewed changes

kritzcreek force-pushed the cache-last-rebuild branch from ed5fa86 to 34fd508 Compare May 8, 2016 21:21

kritzcreek force-pushed the cache-last-rebuild branch from a2adead to 18723c7 Compare May 14, 2016 19:46

kritzcreek force-pushed the cache-last-rebuild branch from 18723c7 to 8e61d9a Compare May 15, 2016 11:46

kritzcreek force-pushed the cache-last-rebuild branch from 8e61d9a to 9bfa4ff Compare May 15, 2016 23:38

kritzcreek added 4 commits May 16, 2016 12:48

Don't add a default import for Prim

0eceb4a

rebuildModule inside ...Make already does that now

only parse a single module when rebuilding

d2b85ac

Now that multiple modules per file are forbidden, we can simplify the parsing code in Rebuild a bit.

remove the cacheSuccess flag

4396720

kritzcreek force-pushed the cache-last-rebuild branch from 9bfa4ff to 4396720 Compare May 16, 2016 10:52

fix docs

41698a8

paf31 reviewed May 22, 2016
View reviewed changes

paf31 merged commit 57e9201 into purescript:0.9 May 22, 2016

kritzcreek deleted the cache-last-rebuild branch May 22, 2016 18:24

Conversation

kritzcreek commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdgarrood commented May 2, 2016

Uh oh!

LiamGoodacre commented May 2, 2016

Uh oh!

kritzcreek commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paf31 commented May 2, 2016

Uh oh!

kritzcreek commented May 2, 2016

Uh oh!

paf31 commented May 2, 2016

Uh oh!

kritzcreek commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garyb commented May 2, 2016

Uh oh!

hdgarrood commented May 2, 2016

Uh oh!

garyb commented May 2, 2016

Uh oh!

texastoland commented May 2, 2016

Uh oh!

garyb commented May 2, 2016

Uh oh!

hdgarrood commented May 2, 2016

Uh oh!

texastoland commented May 2, 2016

Uh oh!

kritzcreek commented May 4, 2016

Uh oh!

garyb commented May 4, 2016

Uh oh!

garyb commented May 4, 2016

Uh oh!

kritzcreek commented May 4, 2016

Uh oh!

garyb commented May 4, 2016

Uh oh!

kritzcreek commented May 6, 2016

Uh oh!

garyb commented May 6, 2016

Uh oh!

kritzcreek commented May 6, 2016

Uh oh!

kritzcreek May 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kritzcreek commented May 8, 2016

Uh oh!

paf31 commented May 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kritzcreek commented May 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paf31 commented May 14, 2016

Uh oh!

coveralls commented May 14, 2016

Uh oh!

kritzcreek commented May 15, 2016

Uh oh!

kritzcreek commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 15, 2016

Uh oh!

paf31 commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kritzcreek commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 16, 2016

kritzcreek commented May 2, 2016 •

edited

Loading

kritzcreek commented May 2, 2016 •

edited

Loading

kritzcreek commented May 2, 2016 •

edited

Loading

kritzcreek May 6, 2016 •

edited

Loading

paf31 commented May 14, 2016 •

edited

Loading

kritzcreek commented May 14, 2016 •

edited

Loading

kritzcreek commented May 15, 2016 •

edited

Loading

paf31 commented May 15, 2016 •

edited

Loading

kritzcreek commented May 15, 2016 •

edited

Loading

paf31 May 22, 2016 •

edited

Loading