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: Cache file error handling on read-only file system. #11946

Merged
merged 1 commit into from Jul 17, 2019

Conversation

@cuki
Copy link
Contributor

commented Jul 4, 2019

What is the purpose of this pull request?

[x] Bug fix (template)

Fixes #11945.

What changes did you make?
Add additional exception for EROFS (when cache file does not exist) next to the already existing ENOENT exception upon trying to delete cache file.

@jsf-clabot

This comment has been minimized.

Copy link

commented Jul 4, 2019

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Jul 4, 2019

@cuki cuki closed this Jul 4, 2019

@cuki cuki reopened this Jul 4, 2019

@cuki cuki changed the title Check cache file exists before deleting it. Fix: Check cache file exists before deleting it. Jul 4, 2019

lib/cli-engine/cli-engine.js Outdated Show resolved Hide resolved
@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

@ilyavolodin @mysticatea Are you confirming this is a bug (you've both commented here but we haven't accepted the issue yet)? Just want to make sure we're all on the same page.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

No, I haven't been able to verify it, but I also don't have an environment to verify it in. I'm going purely on code changes.

@mysticatea

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Same (I have not reproduced it). But because the current code re-throws IO errors except ENOENT, so I think that that's possible. I'm okay if the fix is to ignore more kinds of IO errors.

@cuki cuki changed the title Fix: Check cache file exists before deleting it. Fix: Cache file error handling on read-only file system. Jul 10, 2019

@cuki

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@mysticatea @ilyavolodin I've changed to code to make an exception for EROFS (when cache file does not exist) next to the already existing ENOENT exception within the catch block.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@cuki Thanks, that looks better to me. But I still don't know how to verify this to mark it accepted. I don't have an environment to do so.

@cuki

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@ilyavolodin Here are the steps to reproduce the EROFS: read-only file system, unlink '/src/.eslintcache' error and how to test proposed changes.

1. Create a "project" directory

Which consists out of the following files:

Dockerfile
FROM node:8-stretch

RUN mkdir /src

RUN npm install -g eslint@6.0.1

WORKDIR /src

ENTRYPOINT ["/usr/local/bin/eslint"]
docker-compose.yml
version: '3.4'
services:
  eslint:
    build: .
    read_only: true
    volumes:
    - .:/src:ro,cached
.eslintrc.js
module.exports = {
    "extends": "eslint:recommended"
};
test.js
function hello(){
  console.log('world')
}

hello();

2. Build Docker image

Execute following command inside your "project" directory:

docker-compose build

3. Run ESLint in Docker

Stay in your "project" directory and run:

docker-compose run --rm eslint --config .eslintrc.js test.js

Now you should see something like the following as output:

Error: EROFS: read-only file system, unlink '/src/.eslintcache'
    at Object.fs.unlinkSync (fs.js:1061:18)
    at CLIEngine.executeOnFiles (/usr/local/lib/node_modules/eslint/lib/cli-engine/cli-engine.js:735:20)
    at Object.execute (/usr/local/lib/node_modules/eslint/lib/cli.js:209:111)
    at Object.<anonymous> (/usr/local/lib/node_modules/eslint/bin/eslint.js:78:28)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)

4. Test proposed changes

Change Dockerfile in your "project" directory to be as followed:

FROM node:8-stretch

RUN mkdir /src

RUN npm install -g eslint@6.0.1

# Override relevant file with proposed changes
RUN git clone --single-branch -b patch-1 https://github.com/cuki/eslint /tmp/eslint/ && \
    cp /tmp/eslint/lib/cli-engine/cli-engine.js /usr/local/lib/node_modules/eslint/lib/cli-engine/cli-engine.js

WORKDIR /src

ENTRYPOINT ["/usr/local/bin/eslint"]

Rebuild Docker image (with proposed changes)

docker-compose build

Run ESLint again in Docker:

docker-compose run --rm eslint --config .eslintrc.js test.js

And notice the output, which threw out an error before, now should be:

/src/test.js
  2:3  error  'console' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

@aladdin-add aladdin-add added bug core evaluating and removed triage labels Jul 12, 2019

@cuki

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@ilyavolodin @mysticatea Did either one of you managed to reproduce the issue (and test proposed changes) using the steps from the above (previous) comment? Would be greatly appreciated to get some feedback to see whether this is an issue with my environment or in general. Thanks!

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

Sorry, I was not able to test this, I do not have docker installed, and I currently do not have time to go through installation and configuration of a new environment. Maybe somebody else on the team can do it?

@mysticatea

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

OK, I confirmed it. Thank you!

@mysticatea mysticatea added accepted and removed evaluating labels Jul 14, 2019

@mysticatea
Copy link
Member

left a comment

LGTM, thank you.

@cuki

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

@mysticatea @ilyavolodin Thanks for the quick response and confirming the issue/fix!

@mysticatea mysticatea merged commit cb475fd into eslint:master Jul 17, 2019

9 checks passed

commit-message Commit message follows guidelines
Details
continuous-integration Build #20190710.5 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@NikitaNaumenko NikitaNaumenko referenced this pull request Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.