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

removeFile() Usage #1424

Merged
merged 13 commits into from
Sep 25, 2024
Merged

removeFile() Usage #1424

merged 13 commits into from
Sep 25, 2024

Conversation

bmhan12
Copy link
Contributor

@bmhan12 bmhan12 commented Sep 24, 2024

This PR:

  • Fixes removeFile failure on Windows in slic_macros.cpp unit test by closing the files associated with ifstream and the ofstream member variables of LogStream instances.
  • Checks the result of removeFile in unit tests to verify file has been deleted

Closes issue #1415

@bmhan12 bmhan12 added Core Issues related to Axom's 'core' component Slic Issues related to Axom's 'slic' component labels Sep 24, 2024
// Closes open file streams associated with Slic streams when destructors
// called during slic::finalize().
// Windows _unlink file deletion fails if file is still in use.
#ifdef WIN32
Copy link
Member

Choose a reason for hiding this comment

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

Unless there's a reason non-Windows should not finalize(), let's please remove the guard on WIN32 here and in the slic_macros_parallel.cpp test. If there is a reason to test removeFile() on non-finaliz()ed slic on non-Windows platforms, I would appreciate knowing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guard removed! finalize() call will work on Windows and non-Windows systems, guard was a relic of my testing.

@agcapps
Copy link
Member

agcapps commented Sep 25, 2024

@bmhan12 , thank you for your attention to detail on this.

@bmhan12 bmhan12 force-pushed the bugfix/han12/removeFile_Windows branch from fdc0fed to e528819 Compare September 25, 2024 15:28
Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks @bmhan12 !

@@ -457,7 +456,7 @@ TEST(slic_macros, test_macros_file_output)
std::string no_fmt_expected;
no_fmt_expected += "*****\n[INFO]\n\n Test \n\n ";
no_fmt_expected += __FILE__;
no_fmt_expected += "\n441\n****\n";
no_fmt_expected += "\n440\n****\n";
Copy link
Member

Choose a reason for hiding this comment

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

What is 440 here? Is it a line number?
It might make sense to add a comment about this for future maintenance of these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is 440 here? Is it a line number?

Yes. In a follow-up PR related to #1410, I'll be updating these line numbers to be like slic_macros_parallel to be __LINE__ - offset to make that clear.

@kennyweiss
Copy link
Member

kennyweiss commented Sep 25, 2024

@bmhan12 -- could you please update the documentation for removeFile about it not working on Windows when there are open file handles?

See:

/*!
* \brief Remove the specified file.
* \param filename The name of the file.
* \return 0 on success, -1 on failure. errno can obtain more information
* about the failure.
*/
int removeFile(const std::string& filename);

@rhornung67 rhornung67 merged commit d290434 into develop Sep 25, 2024
13 checks passed
@rhornung67 rhornung67 deleted the bugfix/han12/removeFile_Windows branch September 25, 2024 20:46
@bmhan12 bmhan12 mentioned this pull request Sep 30, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issues related to Axom's 'core' component Slic Issues related to Axom's 'slic' component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants