Skip to content

[envrc] run devbox install to ensure createWrappers is called #1092

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 1 commit into from
Jun 6, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Jun 2, 2023

Summary

Our .envrc today calls shellenv but that doesn't run createWrappers.

If the package's binary may change, then the .envrc environment will not get updated.

How was it tested?

in examples/flakes/php, I did:

  1. devbox generate direnv
  2. php -m | grep ds which prints ds.
  3. edit examples/flakes/php/my-php-flake/flake.nix to remove ds extension
  4. re-run php -m | grep ds. This still prints ds.

Now, for the test steps:

  • cd out of the direnv
  • re-enter the direnv-enabled directory (examples/flakes/php)
  • re-run php -m | grep ds.
    BEFORE: This still prints ds.
    AFTER: no ds is printed.

Note, in Step 4, ideally we would not print ds but for that to happen the direnv would need
to check local flake files for changes. Today, it just checks devbox.json for changes.
Not addressing in this PR (workaround: cd out and back in to the directory)

Copy link
Collaborator Author

savil commented Jun 2, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested review from mikeland73 and mohsenari June 2, 2023 23:58
Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

This is definitely ok to merge, but I wonder if we want devbox shellenv --init-hook --ensure-wrappers (once we change create wrappers to ensure) for perf reasons. This hook gets triggered every time the directory opens so even minor (in my case 30ms when cached) latencies can add up.

@savil
Copy link
Collaborator Author

savil commented Jun 6, 2023

@mikeland86 yeah, that makes sense to me. I agree.

I think its okay to land this since its in the code we generate, and not user-visible .envrc file.

@savil savil force-pushed the savil/envrc-createwrappers branch from 78813a9 to 36865e0 Compare June 6, 2023 23:50
@savil savil merged commit a7aed71 into main Jun 6, 2023
@savil savil deleted the savil/envrc-createwrappers branch June 6, 2023 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants