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
Merged

Fix: Cache file error handling on read-only file system. #11946

merged 1 commit into from Jul 17, 2019

Conversation

cuki
Copy link
Contributor

@cuki cuki 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
Copy link

jsf-clabot commented Jul 4, 2019

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon 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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

cuki 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
Copy link
Member

@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
Copy link
Contributor Author

cuki 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 ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 12, 2019
@cuki
Copy link
Contributor Author

cuki 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
Copy link
Member

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
Copy link
Member

OK, I confirmed it. Thank you!

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 14, 2019
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@cuki
Copy link
Contributor Author

cuki commented Jul 14, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying to delete non-existent .eslintcache on read-only file system
6 participants