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

Knip does not ignore gitignored directory when it has bad permissions #172

Closed
ChristophP opened this issue Jul 25, 2023 · 22 comments
Closed
Labels
bug Something isn't working

Comments

@ChristophP
Copy link

Hi,

I'm using the latest knip 2.17.0 on a Linux machine.

This is my knip config:

{
  "entry": ["src/app.js", "scripts/**/*.js"],
  "project": ["**/*.js"]
}

I have a directory called mongodata in the project root, which contains MongoDB data from a DB that's running in a docker container. Since it's running in the docker container it has different file permission than the rest of the project files.
The mongodata folder is listed in my .gitignore.

Nevertheless when I run knip I get this:
grafik

I even tried adding ignore: ["mongodata/"] to the knip.json, but I still got the error.

@ChristophP ChristophP added the bug Something isn't working label Jul 25, 2023
@ChristophP
Copy link
Author

It can probably be reproduced by running this in an existing knip project

mkdir somedir
chown -R 1400:1400 somedir

@webpro
Copy link
Collaborator

webpro commented Jul 26, 2023

Can't seem to reproduce it here (I'm on macOS).

Not sure if easily fixable by Knip. Perhaps easiest to update config to something like this:

{
  "entry": ["src/app.js", "scripts/**/*.js"],
  "project": ["{src,scripts}/**/*.js"]
}

Would that work for you?

@ChristophP
Copy link
Author

ChristophP commented Jul 26, 2023

Hi @webpro ,

I checked a bit more I there was one thing missing in my previous description to reproduce.

In an existing knip project do

mkdir somedir
sudo chown -R 1400:adm somedir   # change the ownership to user 1400 and group adm
sudo chmod -R 400 somedir        # make the dir only accesible to the owner
echo somedir >> .gitignore    # make sure it's ignored 

npx knip      # crashes due to lacking file permissions on the ignored directory

The fact that it gives this error due to lacking permissions is not unexpected. The weird thing is just that the error still occurs even when the directory is added to the .gitignore. This means that knip must somehow try to scan the directory even though it should be ignored.

Can you reproduce it this way?

@webpro
Copy link
Collaborator

webpro commented Jul 27, 2023

Globbing the file system is done by globby, which in turn uses fast-glob. It is configured to take the .gitignore into account.

I see fast-glob has a suppressErrors: true option. I will think about either using that option or try/catching the error and printing a warning, so Knip's process isn't halted.

@ChristophP
Copy link
Author

ChristophP commented Jul 27, 2023

I see fast-glob has a suppressErrors: true option. I will think about either using that option or try/catching the error and printing a warning, so Knip's process isn't halted.

That would be a workaround. However it seems to me that, if the globbing correctly ignored the items from .gitignore the error shouldn't happen in the first place.
Because, it's no surprise knip crashes when trying to open a directory it doesn't have permissions for. The curious question to me is: Why is knip trying to open that directory in the first place, when it actually should be ignored from the results?

@ChristophP
Copy link
Author

I think I have narrowed down the last call before the error happens. It should be getEntryPathFromManifest function
https://github.com/webpro/knip/blob/main/src/index.ts#L155

@ChristophP
Copy link
Author

getEntryPathFromManifest in turn calls _glob here https://github.com/webpro/knip/blob/main/src/util/modules.ts#L57C2-L57C2 .
It seems like globby still tries to access the directory even when it's gitignored.

Now that I understand that I think you're sugesstion of catching or supressing the error is probably the way to go here.

@ChristophP
Copy link
Author

ChristophP commented Jul 27, 2023

Btw, I modified my knip.json to have this content

{
  "entry": ["src/app.js", "scripts/**/*.js"],
  "project": ["src/**/*.js", "scripts/**/*.js"]
}

and I still get the same error. Even though the offending mongodata/ directory is above src and scripts AND contained in .gitignore. Very strange

/
|- mongodata/
|- src/
|- scripts/

@webpro
Copy link
Collaborator

webpro commented Jul 31, 2023

The exceptions is raised also when only revoking read access (chmod -r dir).

The issue seems to be that globby creates a filter and makes fast-glob look up **/.gitignore to do so. Even when the dir is moved to a/b/c/mongodata you'd get this error.

What could Knip do here:

  • Catching the error and printing some warning would make the user aware, but there would be no solution (other than fixing ownership or removing the dir), since Knip will not return results.
  • Suppressing errors could be a solution in some cases, but very hard to debug situations for others.

A combination of catch-suppress is most user-friendly I guess, it can print a warning and still return useful results. Implementation and performance wise it's ugly.

@ChristophP
Copy link
Author

Ah that makes sense. So globby using doing a recursive lookup for .gitignore files in all subdirectories and crashes when it can't read one.
It seems this is a strange behavior on globbys part, or that at least would be nice if globby had some way of skipping directories it can't read. I added an issue on globby side for this.

In the mean time your suggestions sound good.
What I also found was this option https://github.com/sindresorhus/globby#ignorefiles

If knip somehow exposed this option then a user (like me) could specify his/her own pattern. In this case I could tell knip/globby that my ignore file pattern is simply .gitignore instead of the default **/.gitignore.

@ChristophP
Copy link
Author

Update: Seems like the author of globby considers the current behavior a bug.

@webpro
Copy link
Collaborator

webpro commented Aug 5, 2023

Thanks!

There's currently no way to override the hard-coded **/.gitignore pattern in globby.

Still not sure about that workaround in Knip, but will keep this ticket open.

@ChristophP
Copy link
Author

ChristophP commented Aug 5, 2023

Got it.

My current workaround is to just add read permissions (chmod +r) for all users for the Folder that causes the Bug, which let's me use knip normally.

Knowing that the globby author considers this a bug probably means that it will get fixed at some point.
With workaround exising maybe knip doesn't have to implement its own workaround and just wait until it gets fixed upstream.

Feel free to open or close this issue as you see fit.

@webpro
Copy link
Collaborator

webpro commented Sep 20, 2023

Since the actual issue is upstream, I didn't hear from anyone else, and searching for EACCESS in the globby and fast-glob repos also don't show many issues, I'm going to close this.

@webpro webpro closed this as completed Sep 20, 2023
@ChristophP
Copy link
Author

That's fine. The upstream issue is labelled with "help wanted" so it looks like they are looking for a contribution. It could take a while until it's fixed.

@phiresky
Copy link
Contributor

phiresky commented Nov 2, 2023

I'm hitting the same error. I have a huge /notes directory in my repo that is gitignored and contains all sorts of files. Deep inside there's something not readable by my current user, and knip fails with Error: EACCES: permission denied, scandir on that directory.

The fact that there's no way to work around this in knip makes knip unusable for me which is sad (tried changing the include glob but as written above it doesn't change anything, if something anywhere in the directory structure has no permission, knip won't work. this also seems like a huge perf issue tbh, is it scanning millions of files in my node_modules folder as well?)

@ChristophP
Copy link
Author

@webpro FYI seems like globby fixed this issue upstream sindresorhus/globby#259

@phiresky
Copy link
Contributor

phiresky commented Feb 11, 2024

I glanced at their fix but if I understand correctly it only solves the issue in narrow cases - when the permissionless files are in one of the locations that's in the hardcoded ignore list (not when it's in a gitignore file)

doesn't apply to knip though because we fixed it differently

@ChristophP
Copy link
Author

ChristophP commented Feb 11, 2024

Ah ok, I didn't Look at it in depth yet but thought it was the Kind of fix discussed in this issue.

doesn't apply to knip though because we fixed it differently

Ah so knip rolled it's own solution?

@webpro
Copy link
Collaborator

webpro commented Feb 11, 2024

Ah so knip rolled it's own solution?

@phiresky did! In #426 ❤️

@ChristophP
Copy link
Author

Ah fantastic. So knip doesn't even use globby in this case anymore. 👍

@webpro
Copy link
Collaborator

webpro commented Feb 11, 2024

That PR replaces cq removes globby from the dependency list indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants