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

align exception from file-timestamp functions from different impl. #121

Merged
merged 1 commit into from
Jan 23, 2018
Merged

align exception from file-timestamp functions from different impl. #121

merged 1 commit into from
Jan 23, 2018

Conversation

j-keck
Copy link
Contributor

@j-keck j-keck commented Jan 20, 2018

  • The native implementations to get / set a file-timestamp throws
    a 'java.io.FileNotFoundException' where the Java based implementation
    throws a 'java.nio.file.NoSuchFileException' if the given file
    does not exists.

  • 'IO.getModifiedTimeOrZero' and 'IO.setModifiedTimeOrFalse' handles
    only 'java.io.FileNotFoundException'

  • Map 'NoSuchFileExeption' to 'FileNotFoundException' in the
    Java based implementation, so all implementations throws
    the same error.

fixes sbt/sbt/issues/3894

@eed3si9n eed3si9n added the ready label Jan 20, 2018
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM. Requesting review by @cunei for his thoughts.

@dwijnand dwijnand requested a review from cunei January 22, 2018 08:54
@dwijnand dwijnand changed the base branch from 1.x to 1.1.x January 22, 2018 08:55
@dwijnand dwijnand changed the base branch from 1.1.x to 1.x January 22, 2018 08:55
@dwijnand dwijnand changed the base branch from 1.x to 1.1.x January 22, 2018 08:55
@dwijnand dwijnand changed the base branch from 1.1.x to 1.x January 22, 2018 09:00
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Sorry @j-keck, do you mind rebasing (or cherry-picking) this commit onto the 1.1.x branch? That way hopefully we can merge it for sbt 1.1.1.

@j-keck j-keck changed the base branch from 1.x to 1.1.x January 22, 2018 11:33
@j-keck
Copy link
Contributor Author

j-keck commented Jan 22, 2018

@dwijnand done, this pr is on top of 1.1.x

dwijnand
dwijnand previously approved these changes Jan 22, 2018
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thanks. Re-LGTM from me.

Copy link

@cunei cunei left a comment

Choose a reason for hiding this comment

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

Thanks @j-keck, it looks good apart from a comment I left concerning stack traces.

try {
f
} catch {
case e: NoSuchFileException => throw new FileNotFoundException(e.getFile)
Copy link

Choose a reason for hiding this comment

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

An issue here is that the original stack trace is lost. It is not a big problem since mapNoSuchFileException() is only used once, but if it were used from more than one method, it would not be clear which code path led to the exception. Normally that can be avoided by chaining exceptions; however, it just so happens that FileNotFoundException does not have a Throwable-based constructor, so chaining is a bit more difficult than usual. Still, there is a workaround:

case e: NoSuchFileException => throw new FileNotFoundException(e.getFile) {
  initCause(e)
  override def toString() = {
    val msg = getLocalizedMessage()
    getClass().getSuperclass().getName() + (if (msg == null) "" else (": " + msg))
  }
}

This will be reported as:

java.io.FileNotFoundException: filename
        at ...
Caused by: java.nio.file.NoSuchFileException: filename
        at ...

Where the second exception identifies the original stacktrace. The additional toString() is needed in order to keep the exception identifying itself as a FileNotFoundException in the printout.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can just call initCause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your suggestions.
yes, throw new FileNotFoundException(e.getFile).initCause(e) preserves the stacktrace - thanks:

[error] java.io.FileNotFoundException: /nothing
[error] 	at sbt.internal.io.JavaMilli$.mapNoSuchFileException(Milli.scala:334)
[error] 	at sbt.internal.io.JavaMilli$.getModifiedTime(Milli.scala:322)
[error] 	at sbt.internal.io.Milli$.getModifiedTime(Milli.scala:362)
[error] 	at sbt.io.IO$.getModifiedTime(IO.scala:1143)
...
[error] Caused by: java.nio.file.NoSuchFileException: /nothing
[error] 	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
[error] 	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
[error] 	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
...

should i make this change in a new commit, or amend the actual commit?

Copy link
Member

Choose a reason for hiding this comment

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

As you prefer.

 * The native implementations to get / set a file-timestamp throws
   a 'java.io.FileNotFoundException' where the Java based implementation
   throws a 'java.nio.file.NoSuchFileException' if the given file
   does not exists.

 * 'IO.getModifiedTimeOrZero' and 'IO.setModifiedTimeOrFalse' handles
   only 'java.io.FileNotFoundException'

 * Map 'NoSuchFileExeption' to 'FileNotFoundException' in the
   Java based implementation, so all implementations throws
   the same error.
@j-keck
Copy link
Contributor Author

j-keck commented Jan 23, 2018

done

@dwijnand dwijnand merged commit db7dbd0 into sbt:1.1.x Jan 23, 2018
@dwijnand dwijnand removed the ready label Jan 23, 2018
@dwijnand
Copy link
Member

Thanks, @j-keck.

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.

4 participants