Skip to content

Conversation

@Grimpper
Copy link
Contributor

@Grimpper Grimpper commented Dec 23, 2024

Hello!

This PR adds support for asynchronous execution. Whether to load the environments synchronously or asynchronously can be configured with the variable envrc-async-processing.

This also refactors the envrc lighter to replace it with a mode line indicator. This indicator introduces a new loading state, where a spinner hints buffers that are waiting for the environment to load. The widget can be disabled by setting the envrc-add-to-mode-line-misc-info variable to nil.

The following screencast demonstrates the async processing as well as the new widget:
envrc-async-processing-demo

In the future we could consider implementing a vtable buffer to display running async processes, similarly to how dtached.el displays detached processes.

Regards,
Sergio.

@Grimpper Grimpper force-pushed the support-async-execution branch from 5523ad8 to 999e964 Compare December 24, 2024 09:58
@purcell
Copy link
Owner

purcell commented Dec 24, 2024

Hi, thanks for this. Did you read the discussion in #6 and #53? There are a number of trade-offs around adding this support, and I'm not yet convinced I want to take on async support. As a side comment, changing the lighter code would better be addressed separately: there was discussion of that in #86.

@Grimpper
Copy link
Contributor Author

Hi, thanks for this. Did you read the discussion in #6 and #53? There are a number of trade-offs around adding this support, and I'm not yet convinced I want to take on async support. As a side comment, changing the lighter code would better be addressed separately: there was discussion of that in #86.

I did not know about this discussions. I will take a look. Thanks for pointing them out!

@Grimpper
Copy link
Contributor Author

Grimpper commented Dec 24, 2024

... As a side comment, changing the lighter code would better be addressed separately: there was discussion of that in #86.

I decided to move the status indicator out from the lighter, since that way, it will work with custom mode lines, such as doom-modeline, out of the box. This is the approach that notmuch-indicator.el uses.

@purcell
Copy link
Owner

purcell commented Dec 24, 2024

decided to move the status indicator out from the lighter, since that way, it will work with custom mode lines, such as doom-modeline, out of the box. This is the approach that notmuch-indicator.el uses.

Yeah, not averse to the change itself, just that it's orthogonal to the sync/async change and would be most easily considered/reviewed separately.

@Grimpper
Copy link
Contributor Author

Yeah, not averse to the change itself, just that it's orthogonal to the sync/async change and would be most easily considered/reviewed separately.

Well. I agree that the decision of moving it out of the lighter can be done outside of the async PR, but the status indicator augments the logic to process the loading state, which is an additional feature on top of the async processing.

If this PR gets merged, I think it's important to provide the loading indicator state. It hints the user which buffers are ready to be used with the environment loaded.

I think with this indicator we address some of the concerns described in #6, since the user can know what buffers are waiting for their environment to load.

@Grimpper Grimpper force-pushed the support-async-execution branch 2 times, most recently from 833842b to cab3d37 Compare January 1, 2025 21:00
@Grimpper
Copy link
Contributor Author

Grimpper commented Jan 1, 2025

Yeah, not averse to the change itself, just that it's orthogonal to the sync/async change and would be most easily considered/reviewed separately.

Hello!

I've force pushed some changes to accommodate for the extraction of the status lighter refactor in a separate branch.

There are also some bug fixes to the logic and I've clean up the commits.

In addition I added an option to prompt to query the user before killing already running async processes. I believe this interface solves many concerns about concurrency. For example, if there is a process exporting the environment (eg. envrc-allow) and the user issues envrc-deny, the process that is running will be killed if the user answers yes to the prompt. Similar logic applies to envrc-reload and envrc-allow, the user will be given the option to kill the running processes and the new processes will execute. If, for example, the user would have changed the .envrc file, it provides the option to load the new environment, without worrying about the callbacks.

@cassandracomar
Copy link

I think, if I'm understanding the discussion on those issues correctly, the open question is what to do about subsequent hooks that depend on the environment loaded by envrc. consider a complex lsp setup where the dependencies don't exist in the process environment until after .envrc is loaded into the process environment (e.g. because it's loaded from a nix flake). running a subsequent lsp-mode hook before the envrc hook actually completes results in a broken state where lsp-mode fails to start correctly. in order to handle this seamlessly, we'd need some way to defer loading subsequent hooks until after envrc finishes loading, and such a mechanism doesn't exist at present.

that said, having this feature as default-off and with this caveat properly documented so users know to reload such hooks after the environment loads is probably sufficient. could we add a post-envrc hook for such users, so they can load their dependent hooks after the async process call completes and the environment is loaded? synchronous loading of .envrc while working on many buffers bites me, personally, all the time -- it would be really nice to be able to work on other buffers while I wait for an envrc environment to load/update.

If needed, checking if the envrc directory is allowed, can be implemented with
the following procedures:
```
(defun envrc--allowed-p (env-dir)
  "Return TRUE if ENV-DIR is blocked."
  (let ((allow-hash (envrc--find-allow-hash env-dir))
        (deny-path (concat (or (getenv "XDG_DATA_HOME")
                               (concat (getenv "HOME") "/.local/share"))
                           "/direnv/allow")))
      (locate-file allow-hash
                   (list deny-path))))

(defun envrc--allowed-p (env-dir)
  "Return TRUE if ENV-DIR is blocked."
  (let ((allow-hash (envrc--find-allow-hash env-dir))
        (deny-path (concat (or (getenv "XDG_DATA_HOME")
                               (concat (getenv "HOME") "/.local/share"))
                           "/direnv/allow")))
      (locate-file allow-hash
                   (list deny-path))))
```
@Grimpper Grimpper force-pushed the support-async-execution branch from cab3d37 to 71f6797 Compare January 10, 2025 19:34
@tylerjl
Copy link

tylerjl commented Feb 24, 2025

[...] having this feature as default-off and with this caveat properly documented so users know to reload such hooks after the environment loads is probably sufficient.

Default-off with caveats listed under the boolean variable seems pretty reasonable? Assuming that the requisites for async execution don't pollute the non-async paths too much and potentially make the default path buggy.

could we add a post-envrc hook for such users, so they can load their dependent hooks after the async process call completes and the environment is loaded?

Adding a hook that's called once async envrc is complete sounds really nice. For what it's worth, I'm sort of doing this today by adding a variable watch onto envrc--status and reacting to when the status changes, and it works as expected (for example, my LSP executables aren't present outside of direnv, but show up after entering it. It's sort of manual since I have to wait for the status to change, check that it's done, and then invoke eglot, but that's the tradeoff with the async path).

+1 to separating out the lighter/modeline changes into a separate PR, though.

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.

4 participants