Temporary files on Windows #63

Closed
utelle opened this Issue Jul 6, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@utelle

utelle commented Jul 6, 2016

This issue is somewhat related to issue #53, but that one has a misleading subject.

On Windows the current default implementation of lxw_tmpfile (which uses tmpfile under the hood) can impose a real problem. According to the documentation tmpfile always creates temporary files in the root directory. However, the root directory is not accessible for users without elevated rights. This makes libxlsxwriter fail and results in zero-length Excel files.

The alternative implementation might be ok on Linux systems, but doesn't work on Windows. And a hard-coded path isn't a good idea anyway.

In the meantime I implemented a Windows and MSVC specific solution and of course I can post it here.

Additionally you can find under the following link a generic implementation: An all-singing, all-dancing C function to create a temporary file. Maybe this could be a generic solution for creating temporary files.

@jmcnamara jmcnamara self-assigned this Jul 6, 2016

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 6, 2016

Owner

Hi Ulrich,

According to the documentation tmpfile always creates temporary files in the root directory.

That incredible piece of software engineering has been a thorn in my side for years. The Perl and Python versions of this library have options to override the default temp directory.

In the meantime I implemented a Windows and MSVC specific solution and of course I can post it here.

Yes. That might help someone else who is struggling with this problem.

Maybe this could be a generic solution for creating temporary files.

I'm little reticent to include something like this but there may be no choice for Windows users. I'll look into adding it on a branch for testing and take it from there.

John

Owner

jmcnamara commented Jul 6, 2016

Hi Ulrich,

According to the documentation tmpfile always creates temporary files in the root directory.

That incredible piece of software engineering has been a thorn in my side for years. The Perl and Python versions of this library have options to override the default temp directory.

In the meantime I implemented a Windows and MSVC specific solution and of course I can post it here.

Yes. That might help someone else who is struggling with this problem.

Maybe this could be a generic solution for creating temporary files.

I'm little reticent to include something like this but there may be no choice for Windows users. I'll look into adding it on a branch for testing and take it from there.

John

@utelle

This comment has been minimized.

Show comment
Hide comment
@utelle

utelle Jul 8, 2016

Hi John,

sorry for the delay. I was busy in my job.

In the meantime I implemented a Windows and MSVC specific solution and of course I can post it here.

Yes. That might help someone else who is struggling with this problem.

I'll try to provide a variant that might be usable not only with MSVC. Not sure whether I succeed. I'll post the code early next week.

In fact, my implementation is similar to the generic implementation I quoted - just without the unneeded bits and pieces.

utelle commented Jul 8, 2016

Hi John,

sorry for the delay. I was busy in my job.

In the meantime I implemented a Windows and MSVC specific solution and of course I can post it here.

Yes. That might help someone else who is struggling with this problem.

I'll try to provide a variant that might be usable not only with MSVC. Not sure whether I succeed. I'll post the code early next week.

In fact, my implementation is similar to the generic implementation I quoted - just without the unneeded bits and pieces.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 8, 2016

Owner

sorry for the delay. I was busy in my job.

No problem. We all have jobs and probably none of them relate to creating Excel files for a living.

I'll try to provide a variant that might be usable not only with MSVC. Not sure whether I succeed. I'll post the code early next week.

In fact, my implementation is similar to the generic implementation I quoted - just without the unneeded bits and pieces.

I'm going to put together a branch with the above library as an optional make option on Unix and as the default on Windows. I'll ping you when I've implemented and tested it a bit. If it looks stable I'll merge it to master.

Owner

jmcnamara commented Jul 8, 2016

sorry for the delay. I was busy in my job.

No problem. We all have jobs and probably none of them relate to creating Excel files for a living.

I'll try to provide a variant that might be usable not only with MSVC. Not sure whether I succeed. I'll post the code early next week.

In fact, my implementation is similar to the generic implementation I quoted - just without the unneeded bits and pieces.

I'm going to put together a branch with the above library as an optional make option on Unix and as the default on Windows. I'll ping you when I've implemented and tested it a bit. If it looks stable I'll merge it to master.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 10, 2016

Owner

I've pushed a version using tmpfileplus as the default tmpfile handler to the following branch: https://github.com/jmcnamara/libxlsxwriter/tree/tmpfileplus

I need to add some more documentation and fix the Mac cocoapod files but if you get a chance you can test it to see if it works for you.

Owner

jmcnamara commented Jul 10, 2016

I've pushed a version using tmpfileplus as the default tmpfile handler to the following branch: https://github.com/jmcnamara/libxlsxwriter/tree/tmpfileplus

I need to add some more documentation and fix the Mac cocoapod files but if you get a chance you can test it to see if it works for you.

@utelle

This comment has been minimized.

Show comment
Hide comment
@utelle

utelle Jul 11, 2016

I will test it within the next few days.

I inspected the code of tmpfileplus. The approach seems to be more or less the same as what I had implemented in my local version. That is, I think I can forego to publish my own implementation.

utelle commented Jul 11, 2016

I will test it within the next few days.

I inspected the code of tmpfileplus. The approach seems to be more or less the same as what I had implemented in my local version. That is, I think I can forego to publish my own implementation.

@utelle

This comment has been minimized.

Show comment
Hide comment
@utelle

utelle Jul 11, 2016

I tested the new implementation. It works as expected.

There is only one thing that I don't like in tmpfileplus: as default it uses the filename prefix tmp. making the random part of the filename in fact the filename extension. In case the temporary files are not deleted automatically for some reason or need to be inspected during debugging, this makes it difficult to locate the files. I would prefer a default prefix tmp_ or tmp- or libxlsxwriter or no prefix at all. This can easily be achieved by calling tmpfileplus in function lxw_tmpfile with a not-NULL prefix parameter like

    return tmpfileplus(tmpdir, "libxlsxwriter", NULL, 0);

Alternatively, the default value in tmpfileplus could be changed.

utelle commented Jul 11, 2016

I tested the new implementation. It works as expected.

There is only one thing that I don't like in tmpfileplus: as default it uses the filename prefix tmp. making the random part of the filename in fact the filename extension. In case the temporary files are not deleted automatically for some reason or need to be inspected during debugging, this makes it difficult to locate the files. I would prefer a default prefix tmp_ or tmp- or libxlsxwriter or no prefix at all. This can easily be achieved by calling tmpfileplus in function lxw_tmpfile with a not-NULL prefix parameter like

    return tmpfileplus(tmpdir, "libxlsxwriter", NULL, 0);

Alternatively, the default value in tmpfileplus could be changed.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 11, 2016

Owner

I've left the prefix as it is. If it becomes a real issue for someone I'll change it.

I've merged the tmpfile changes down to master and pushed them in the 0.4.1 release: http://libxlsxwriter.github.io/changes.html

Thanks for the report. Closing.

Owner

jmcnamara commented Jul 11, 2016

I've left the prefix as it is. If it becomes a real issue for someone I'll change it.

I've merged the tmpfile changes down to master and pushed them in the 0.4.1 release: http://libxlsxwriter.github.io/changes.html

Thanks for the report. Closing.

@jmcnamara jmcnamara closed this Jul 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment