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

fix: add helper plugin to keep paths within sandbox #160

Merged
merged 8 commits into from
Jan 4, 2024

Conversation

vpanta
Copy link
Contributor

@vpanta vpanta commented Sep 14, 2023

Fixes #58.

Adds a "bazel-sandbox" helper plugin when using Esbuild under Bazel.

It is on by default but can be opted-out by setting bazel_sandbox_plugin = False on an esbuild target.

This PR also adds js_log_level and esbuild_log_level attributes. When js_log_level is debug, the bazel-sandbox plugin will print out resolution changes it makes.


Type of change

  • Bug fix (change which fixes an issue)

For changes visible to end-users

  • Relevant documentation has been updated
  • Suggested release notes are provided below:

Addition of an automatic plugin ensuring esbuild resolution remains within the sandbox.

Test plan

  • Covered by existing test cases
  • Manual testing; please provide instructions so we can reproduce:

Manually tested changing log levels and disabling the new plugin by setting bazel_sandbox_plugin = False. A follow-up with more testing for this plugin would be good.

@vpanta vpanta mentioned this pull request Sep 14, 2023
helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
@jbedard
Copy link
Member

jbedard commented Sep 18, 2023

@thesayyn and I discussed this and overall agree with what you're suggesting. I think this should be setup 100% of the time by the launcher like you said.

In one sense it would be nice to avoid more node since one of the main advantages of esbuild is the use of go, but when we're already launching esbuild from node I don't think this adds any harm (we're not adding another node process being launched etc).

helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@vpanta vpanta left a comment

Choose a reason for hiding this comment

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

Thanks for the review. I attempted an initial stab at integrating it with launcher.js, but not very sure if I did it in an acceptable way (or fully if it works yet).

I agree that I'd prefer this work within esbuild instead, and while I was hesitant to add a JS resolver call for every single resolution call, having the assurance that it plays better with the sandbox seemed worthwhile.

helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
helpers/plugins/sandbox.js Outdated Show resolved Hide resolved
@jbedard
Copy link
Member

jbedard commented Nov 27, 2023

@vpanta seeing the sandbox issue again reminded me of this PR... did you have a chance to look at the latest comments or have any other updates?

@vpanta
Copy link
Contributor Author

vpanta commented Nov 28, 2023

@vpanta seeing the sandbox issue again reminded me of this PR... did you have a chance to look at the latest comments or have any other updates?

shoot, sorry, I spaced on this PR. Let me see what I can do with it today.

@vpanta
Copy link
Contributor Author

vpanta commented Dec 18, 2023

ugh, sorry again, it's been a crazy time but I am actively working on this PR right now and am hoping to settle it this week.

@vpanta vpanta force-pushed the vpanta/sandbox-plugin branch from c160e57 to f60de04 Compare December 18, 2023 18:16
@vpanta
Copy link
Contributor Author

vpanta commented Dec 19, 2023

Ok, the examples are all compiling and I've verified the plugin is being used, so I think this is good for review again.

…bug logs and the ability to set the JS and esbuild log levels
@gregmagolan gregmagolan dismissed thesayyn’s stale review January 4, 2024 21:59

All bits are addressed

@gregmagolan
Copy link
Member

gregmagolan commented Jan 4, 2024

@vpanta I pushed a few changes and updated the description to get this ready for landing

@gregmagolan gregmagolan force-pushed the vpanta/sandbox-plugin branch from 3f9a4ab to cfc9004 Compare January 4, 2024 22:09
Copy link
Member

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

🚀

@gregmagolan gregmagolan merged commit e39ac34 into aspect-build:main Jan 4, 2024
21 checks passed
@vpanta
Copy link
Contributor Author

vpanta commented Jan 10, 2024

Big thanks @gregmagolan to getting this over the line!

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

Successfully merging this pull request may close these issues.

esbuild leaves the sandbox
4 participants