Skip to content

[WIP] Throw Exception if results directory cannot be created or file cannot be written #2188

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tkuchida
Copy link
Member

@tkuchida tkuchida commented May 12, 2018

Fixes #2181.

Brief summary of changes

  • Catch errno == ENOENT if results directory cannot be created.
  • Throw Exception from Storage if file cannot be opened for writing.

Testing I've completed

Ran example setup file that attempted to write results to C:\Program Files:

InverseKinematicsTool Failed: Storage: Failed to open file 'C:\Program Files/ik_marker_errors.sto' for writing.
Verify that the destination directory is writable.
        Thrown at Storage.cpp:2746 in print().

Looking for feedback on...

CHANGELOG.md (choose one)

  • no need to update because... bug fix.
  • updated...

The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.


This change is Reviewable

@tkuchida tkuchida changed the title Throw Exception if results directory cannot be created or file cannot be written [WIP] Throw Exception if results directory cannot be created or file cannot be written May 12, 2018
Copy link
Member

@chrisdembia chrisdembia left a comment

Choose a reason for hiding this comment

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

Would it make sense to put the check OPENSIM_THROW_IF(errno == ENOENT, UnableToCreateDirectory, aDir); inside IO::makeDir()?

@chrisdembia
Copy link
Member

The line number in this exception from the AppVeyor log is suspicious:

Exception:
4466  Unable to create directory 'twoMusclesOnBlock_ResultsCMC'.
4467  Thrown at CMCTool.cpp:831 in run().
4468  line= 1978645904

I don't think any of our source code files have that many lines.

@tkuchida
Copy link
Member Author

tkuchida commented May 13, 2018

@aseth1 Do you want these exceptions thrown from IO.cpp instead , or are there scenarios in which a consumer would want to notify the user but proceed anyway ? (Edit: a caller could still try/catch the exception if it has a way of recovering from the error.)

@tkuchida
Copy link
Member Author

The line number in this exception from the AppVeyor log is suspicious:

I added the UnableToCreateDirectory exception following the example of FileIsEmpty and AFAIK I'm calling it the same way, so if UnableToCreateDirectory is wrong then the other Exceptions in FileAdapter.h may be wrong as well. This is a separate issue and is not responsible for the tests failing.

@tkuchida
Copy link
Member Author

Would it make sense to put the check OPENSIM_THROW_IF(errno == ENOENT, UnableToCreateDirectory, aDir); inside IO::makeDir()?

It depends what you mean by "would it make sense". The current interface of IO::makeDir() is the following:

/**
* Create a directory. Potentially platform dependent.
* @return int 0 on success, EEXIST or other error condition
*/

so it seems like the problem is that the callers are ignoring the return value/error condition, not that makeDir() has been implemented incorrectly.

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.

IO Exception isn't written to messages window
2 participants