tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

fix mktemp linker warnings when using GNU linker #132

Closed stephen-sorley closed 4 years ago

stephen-sorley commented 4 years ago

Use mkdtemp() to create a temporary directory with a unique name, then add a constant filename to the path after that. This allows us to construct a unique path without any race conditions, and without having to use mktemp().

Hopefully doing it this way avoids the issues you were having with getting mkstemp() to work in #43.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.009%) to 83.31% when pulling c366d04b8ee035c2139b234d2bd5af9b715118fa on stephen-sorley:fix-mktemp into 809af56b150f73e9cad590b256979ff528ea6b21 on tbeu:master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.02%) to 82.837% when pulling e0749d2a41fe567f90b80a38591714fce9bc0df6 on stephen-sorley:fix-mktemp into d255262c794a56d6f1c6dfca3e3025e7865c22f3 on tbeu:master.

tbeu commented 4 years ago

@svillemot The changes look very good to me now. Before merging I'd like to get your feedback. Do you see any issues with mkdtemp on Debian platforms? Should we enable mkdtemp only for POSIX systems? Thanks a lot.

svillemot commented 4 years ago

No problem for using mkdtemp() on Debian. But you may indeed have to disable it for non-POSIX systems (for example, I don’t think it is available under MinGW).

stephen-sorley commented 4 years ago

@tbeu Any further changes that you need me to make to this?

What platforms do you support besides Windows/Linux/macOS? Apart from Windows, looks like AIX is the main one that's missing mkdtemp: http://lists.llvm.org/pipermail/llvm-dev/2010-August/034081.html

Solaris provides it, however: https://docs.oracle.com/cd/E36784_01/html/E36874/mkdtemp-3c.html

tbeu commented 4 years ago

@tbeu Any further changes that you need me to make to this?

No, it all looks fine. I only was reluctant to merge since @svillemot mentioned MinGW. I need to give it one more try after I observed no issues when I tested it locally last time.

tbeu commented 4 years ago

I only was reluctant to merge since @svillemot mentioned MinGW. I need to give it one more try after I observed no issues when I tested it locally last time.

I rebased (and force-pushed) your branch to confirm that it builds with MinGW.

What platforms do you support besides Windows/Linux/macOS? Apart from Windows, looks like AIX is the main one that's missing mkdtemp: http://lists.llvm.org/pipermail/llvm-dev/2010-August/034081.html

I have no chance to build on AIX. I believe we should exclude it from the two readme files, where AIX is mentioned as supported platform. OK for you?

stephen-sorley commented 4 years ago

Yeah, excluding AIX sounds good to me. I don't have any way to test on it, either.

tbeu commented 4 years ago

Thanks for the pull request.