Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

Make sure files are readable when traversing source files. #423

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

vic
Copy link
Contributor

@vic vic commented Sep 1, 2018

Mill was trying to read all files found under the source directory to
create a digest for each of them. This was causing an error for broken
symlinks.

At first I believed temporary files should be ignored to avoid this problem,
and asked at the gitter channel how to go about this, but overriding the sources
task as suggested
by @lihaoyi didn't actually help, because it was T.sources macro who was actually
triggering the exception.

So, on a simple scala project, editing a file with Emacs, creates a link file, like:

vic@oeiuwq ~/h/foo> ls -la foo/src/
total 8
drwxr-xr-x  4 vic  staff  128 Sep  1 12:23 .
lrwxr-xr-x  1 vic  staff   22 Sep  1 12:23 .#hello.scala -> vic@oeiuwq.local.10748
drwxr-xr-x  3 vic  staff   96 Sep  1 12:22 ..
-rw-r--r--  1 vic  staff   12 Sep  1 12:22 hello.scala

This patch only makes sures that the files (or the symlink here) is actually
readable before trying to digest it.

Fixes #402

Mill was trying to read all files found under the source directory to
create a digest for each of them. This was causing an error for broken
symlinks.

At first I believed temporary files should be ignored to avoid this problem,
and asked at the gitter channel how to go about this, but overriding the `sources`
task as [suggested](https://gitter.im/lihaoyi/mill?at=5ad6cd801130fe3d36eb7655)
by @lihaoyi didn't actually help.

on a simple scala project, editing a file with Emacs, creates a link file, like:

```
vic@oeiuwq ~/h/foo> ls -la foo/src/
total 8
drwxr-xr-x  4 vic  staff  128 Sep  1 12:23 .
lrwxr-xr-x  1 vic  staff   22 Sep  1 12:23 .#hello.scala -> vic@oeiuwq.local.10748
drwxr-xr-x  3 vic  staff   96 Sep  1 12:22 ..
-rw-r--r--  1 vic  staff   12 Sep  1 12:22 hello.scala
```

So this patch only makes sures that the files (or the symlink here) is actually
readable before trying to digest it.

Fixes com-lihaoyi#402
@lihaoyi
Copy link
Member

lihaoyi commented Sep 2, 2018

Perhaps using a try-catch around the else block would be a better solution? If any IOException appears here, just do nothing

@vic
Copy link
Contributor Author

vic commented Sep 5, 2018

@lihaoyi Hm, not sure if using a try-catch is the way we should go. I mean, since we are trying to create an InputStream for the file and read from it, we should at least check if the file is readable (which is what this PR does), but if then we try to read it and somehow there's an error reading it (say it's on a network filesystem and network goes down), I believe we should let the IOException go and not just ignore it.

Do you have a strong opinion about using the try-catch ?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 5, 2018

No i don't have strong opinions, this looks good

@lihaoyi lihaoyi merged commit ec5f843 into com-lihaoyi:master Sep 5, 2018
@lefou lefou added this to the 0.2.8 milestone Apr 29, 2019
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.

Do not compile hidden files
3 participants