Skip to content

Relocate config file to temp directory #338

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

azzurro
Copy link

@azzurro azzurro commented Apr 26, 2025

The config file was previously stored in the project eslint directory,
which could lead to write error in sandbox environments (like Bazel).
This pull request relocates the config file to the OS's temporary directory,
namespaced by a hash of the project eslint directory.

azzurro added 3 commits April 26, 2025 15:34
The config file was previously stored in the project eslint directory,
which could lead to write error in sandbox environments (like Bazel).
This commit relocates the config file to the OS's temporary directory,
namespaced by a hash of the project eslint directory.
@mantoni
Copy link
Owner

mantoni commented Apr 29, 2025

Thank you for the PR 🙏. I'll find time to test this tomorrow.

@mantoni
Copy link
Owner

mantoni commented Apr 30, 2025

Can you have a look at the failing build please?

azzurro added 2 commits May 2, 2025 14:30
The configFile function in the daemon tests was being called with a base
object instead of the resolver mock object. This commit fixes this issue by
using the resolverMock object in all calls to the configFile function in the
daemon tests.
Use path.join to construct the config file path, ensuring
compatibility across different operating systems by handling path
separators correctly.
@azzurro
Copy link
Author

azzurro commented May 2, 2025

The windows_fix branch might fix the Windows issue.
Let me know if you'd like me to proceed with the merge. I currently have no way to test these changes since Windows environment isn't available to me, and I'd prefer not to merge without testing.

@mantoni
Copy link
Owner

mantoni commented May 5, 2025

I don't have a windows machine to test this either. Sorry. You can simply update this PR or open another one and I'll trigger the builds.

azzurro added 3 commits May 6, 2025 14:12
The file name generated for the config was invalid for some OS's. It
contained a leading ".", which is valid on *nix, but not on Windows.

Also, replaced backslashes with forward slashes in the config SHA256 hash to
maintain consistency.
The config file path generation was not platform independent causing
cache misses on Windows. By using absolute paths, the cache invalidation
works correctly.
On Windows, `path.resolve` returns backslash as path separator.
This commit replaces backslash to slash for consistent file name.
@azzurro
Copy link
Author

azzurro commented May 6, 2025

Tested in a virtual machine with Cygwin.
Last commit is: #d61de93

https://github.com/azzurro/eslint_d.js/actions/runs/14859174014

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.

2 participants