Skip to content
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

Don't restart for removed or renamed modules #290

Merged

Conversation

9999years
Copy link
Member

@9999years 9999years commented Jun 7, 2024

Ghcid crashes when modules are removed/renamed because all it can do to the underlying GHCi session is :reload, and a GHCi bug means that the GHCi session will crash when this happens.

Because ghciwatch tracks which files/modules are loaded in the session, it can accurately :unadd files from the session when they're removed on disk, without requiring the session be restared.

This should speed up development.

I'm going to cut 1.0 with this while we're at it.

  • Labeled the PR with patch, minor, or major to request a version bump when it's merged.
  • Updated the user manual in docs/.
  • Added integration / regression tests in tests/.

@9999years 9999years added the major Breaking changes label Jun 7, 2024
Copy link

linear bot commented Jun 7, 2024

@9999years 9999years force-pushed the rebeccat/dux-2336-remove-modules-instead-of-restarting-ghci branch 4 times, most recently from 78df0b3 to 101cb5a Compare June 10, 2024 23:12
@9999years 9999years marked this pull request as ready for review June 10, 2024 23:15
@9999years 9999years force-pushed the rebeccat/dux-2336-remove-modules-instead-of-restarting-ghci branch from 101cb5a to d02745f Compare June 10, 2024 23:15
@9999years 9999years requested a review from Gabriella439 June 10, 2024 23:19
.await
.expect("ghciwatch loads ghci");

session.fs_mut().disable_load_bearing_sleep();
Copy link
Member Author

Choose a reason for hiding this comment

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

This API is clumsy, the load bearing sleep value should be tied to the particular Fs instance, not shared.

modules
.iter()
.map(|path| {
self.module_import_name(show_paths, path)
Copy link
Member Author

Choose a reason for hiding this comment

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

These ModuleSet APIs are showing some strain, but it's not clear what to replace them with. I tried a few designs and didn't like the results.

Gabriella439
Gabriella439 previously approved these changes Jun 10, 2024
src/ghci/mod.rs Show resolved Hide resolved
tests/globs.rs Outdated Show resolved Hide resolved
@9999years 9999years force-pushed the rebeccat/dux-2336-remove-modules-instead-of-restarting-ghci branch from d02745f to 26cf79b Compare June 11, 2024 17:06
Ghcid crashes when modules are removed/renamed because all it can do to
the underlying GHCi session is `:reload`, and a GHCi bug [1] means that
the GHCi session will crash when this happens.

Because ghciwatch tracks which files/modules are loaded in the session,
it can accurately `:unadd` files from the session when they're removed
on disk, without requiring the session be restared.

This should speed up development.

[1]: https://gitlab.haskell.org/ghc/ghc/-/issues/11596
@9999years 9999years force-pushed the rebeccat/dux-2336-remove-modules-instead-of-restarting-ghci branch from 26cf79b to d66522a Compare June 11, 2024 17:07
@9999years 9999years requested a review from Gabriella439 June 11, 2024 17:07
@9999years 9999years enabled auto-merge (squash) June 11, 2024 17:28
@9999years 9999years requested a review from atruslow June 11, 2024 18:44
Copy link
Contributor

@atruslow atruslow left a comment

Choose a reason for hiding this comment

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

LGTM!

@9999years 9999years merged commit bf25327 into main Jun 11, 2024
38 checks passed
@9999years 9999years deleted the rebeccat/dux-2336-remove-modules-instead-of-restarting-ghci branch June 11, 2024 18:50
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

3 participants