-
Notifications
You must be signed in to change notification settings - Fork 89
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
Implement mkstemp
correctly for working against MSWindows
#110
Comments
This reverts commit d63a1e7. In d63a1e7, @JunyuanChen fixed a long standing compilation warning by replacing `tempnam` with `mkstemp` in `epdfinfo`. However, the `mkstemp` implementation does not work correctly on MS Windows (probably because the path template is "wrong" for Windows). I am reverting the commit and opening a new issue #110 to track the correct implementation of `mkstemp` (or equivalent) on MS Windows. It would also be great to add a test against Windows CI to open a PDF and check that the operation completes successfully. Closes: #101
I am not on Windows so I cannot test it, but according to
So according to c) Maybe the following patch would give the correct template? (against commit d63a1e7) --- a/server/epdfinfo.c
+++ b/server/epdfinfo.c
@@ -347,6 +347,6 @@
static char*
mktempfile()
{
- char template[] = "/tmp/epdfinfoXXXXXX";
+ char template[] = P_tmpdir "/epdfinfoXXXXXX";
char *filename = malloc(sizeof(template));
memcpy(filename, template, sizeof(template)); |
Yes, I think the same. I'll push the change to a branch today and ask
Windows users to test it. If it works we can merge it in.
…On Mon, May 9, 2022, 10:51 PM JunyuanChen ***@***.***> wrote:
I am not on Windows so I cannot test it, but according to man 3 tempnam
it says:
Attempts to find an appropriate directory go through the following steps:
a) In case the environment variable TMPDIR exists and contains the name of an appropriate directory, that is used.
b) Otherwise, if the dir argument is non-NULL and appropriate, it is used.
c) Otherwise, P_tmpdir (as defined in <stdio.h>) is used when appropriate.
d) Finally an implementation-defined directory may be used.
So according to c) P_tmpdir contains the "standard" temporary path on the
target system.
I have tested on glibc 2.35 on Linux, and P_tmpdir is "/tmp".
Maybe the following patch would give the correct template? (against commit
d63a1e7
<d63a1e7>
)
--- a/server/epdfinfo.c+++ b/server/epdfinfo.c@@ -347,6 +347,6 @@
static char*
mktempfile()
{- char template[] = "/tmp/epdfinfoXXXXXX";+ char template[] = P_tmpdir "/epdfinfoXXXXXX";
char *filename = malloc(sizeof(template));
memcpy(filename, template, sizeof(template));
—
Reply to this email directly, view it on GitHub
<#110 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUUHVGZW73XHMPKOMYJGLVJHFMZANCNFSM5VP6AVCQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
It works. FYI. In windows 10, the temp file goes to "c:/Users/username/AppData/Local/Temp/pdf-tools-"xxx as expected. |
Thank you for the confirmation! I will make the change today! |
Sorry. It is my fault. The issue is still there. The different is that the temp dir is created now. However, it reports "Unable to create temporary file". |
:( re-opening this ticket and reverting the commit 37bbe86 |
This reverts commit 37bbe86. As explained by @ShuguangSun (re: testing the change on Windows) on > Sorry. It is my fault. The issue is still there. The different is > that the temp dir is created now. However, it reports "Unable to > create temporary file". Reopens: #110
At Windows 10, It seems no need of the path to temp dir.
|
Okay. Let me test this on Sunday and then I'll merge it in.
…On Thu, May 12, 2022, 8:40 AM ShuguangSun ***@***.***> wrote:
At Windows 10, It seems no need of the path to temp dir.
char template[] = "epdfinfoXXXXXX";
—
Reply to this email directly, view it on GitHub
<#110 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUUHSSWVCU5RERBP7CHN3VJT34XANCNFSM5VP6AVCQ>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
This reverts commit fedd930. It should be possible to use the filename as generated by `mkstemp`, the problem was that we were using the wrong path-separator. So the diff from fedd930 is as follows: - char template[] = P_tmpdir "/epdfinfoXXXXXX"; + char template[] = P_tmpdir "epdfinfoXXXXXX"; I have tested the change Mac OSX, and should in theory work correctly on Linux as well. It is tested on Windows 10 by @ShuguangSun. Closes: #110
This reverts commit fedd930. It should be possible to use the filename as generated by `mkstemp`, the problem was that we were using the wrong path-separator. So the diff from fedd930 is as follows: - char template[] = P_tmpdir "/epdfinfoXXXXXX"; + char template[] = P_tmpdir "epdfinfo_XXXXXX"; I have tested the change Mac OSX, and should in theory work correctly on Linux as well. It is tested on Windows 10 by @ShuguangSun. Closes: #110
I've pushed the final version of this change to I have confirmed that it's working on MacOS, but I need it to be tested on Windows and Ubuntu + Fedora at the minimum. Here is the list of test-cases that should be executed:
If all these steps work, then the change is working correctly on your operation system. |
At Windows 10, It seems no need of the path to temp dir. char template[] = "epdfinfoXXXXXX"; |
If you run |
It print |
This reverts commit fedd930. It should be possible to use the filename as generated by `mkstemp`, the problem was that we were using the wrong path-separator. So the diff from fedd930 is as follows: @@ -338,7 +338,11 @@ strchomp (char *str) return str; } - +#if HAVE_W32 +#define MKTEMP_SEPARATOR "\\" +#else +#define MKTEMP_SEPARATOR "/" +#endif /** * Create a new, temporary file and returns its name. * @@ -347,7 +351,7 @@ strchomp (char *str) static char* mktempfile() { - char template[] = P_tmpdir "epdfinfo_XXXXXX"; + char template[] = P_tmpdir MKTEMP_SEPARATOR "epdfinfo_XXXXXX"; I have tested the change on Mac OSX and Linux. It is tested on Windows 10 by @ShuguangSun. Closes: #110
This reverts commit fedd930. It should be possible to use the filename as generated by `mkstemp`, the problem was that we were using the wrong path-separator. So the diff from fedd930 is as follows: @@ -338,7 +338,11 @@ strchomp (char *str) return str; } - +#if HAVE_W32 +#define MKTEMP_SEPARATOR "\\" +#else +#define MKTEMP_SEPARATOR "/" +#endif /** * Create a new, temporary file and returns its name. * @@ -347,7 +351,7 @@ strchomp (char *str) static char* mktempfile() { - char template[] = P_tmpdir "epdfinfo_XXXXXX"; + char template[] = P_tmpdir MKTEMP_SEPARATOR "epdfinfo_XXXXXX"; I have tested the change on Mac OSX and Linux. It is tested on Windows 10 by @ShuguangSun. Closes: #110
Hi @ShuguangSun and others on this thread, I've pushed the following change to the
I've tested it on Mac OSX and on Linux, and it seems to be working correctly. Can you please test it on Windows? If it's working correctly I will merge the change in and close this ticket. Further, it would be great if other participants in this thread can confirm that the change works on their machines as well. Test steps are described here: #110 (comment) |
It does'nt work on windows. As it pointed above It works well if it changes to |
Does it work with
I am not sure where it goes. This is also why I'm not keen on merging |
It doesn't work. The same error will be printed. |
Difference from 37bbe86: Set temporary directory path in runtime using environment variable: "TMP" for Windows and "TMPDIR" for Unix-like. This is because "P_tmpdir" does not expand correctly on Windows. Temp filename examples: - Windows 11: C:\Users\<username>\AppData\Local\Temp\epdfinfo_xxxxxx - MSYS2 on Windows 11: <path_to_msys2>\tmp\epdfinfo_xxxxxx - macOS Ventura:/var/folders/xx/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/epdfinfo_xxxxxx Closes vedang#110
Difference from 37bbe86: Set temporary directory path in runtime using environment variable: "TMP" for Windows and "TMPDIR" for Unix-like. This is because "P_tmpdir" does not expand correctly on Windows. Temp filename examples: - Windows 11: C:\Users\<username>\AppData\Local\Temp\epdfinfo_xxxxxx - MSYS2 on Windows 11: <path_to_msys2>\tmp\epdfinfo_xxxxxx - macOS Ventura:/var/folders/xx/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/epdfinfo_xxxxxx Closes vedang#110
Difference from 37bbe86: Set temporary directory path in runtime using environment variable: "TMP" for Windows and "TMPDIR" for Unix-like. This is because "P_tmpdir" does not expand correctly on Windows. Temp filename examples: - Windows 11: C:\Users\<username>\AppData\Local\Temp\epdfinfo_xxxxxx - MSYS2 on Windows 11: <path_to_msys2>\tmp\epdfinfo_xxxxxx - macOS Ventura:/var/folders/xx/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/epdfinfo_xxxxxx Closes vedang#110
Difference from 37bbe86: Set temporary directory path in runtime using environment variable: "TMP" for Windows and "TMPDIR" for Unix-like. This is because "P_tmpdir" does not expand correctly on Windows. If the environment variable is not set, falls back to "P_tmpdir". Temp filename examples: - Windows 11: C:\Users\<username>\AppData\Local\Temp\epdfinfo_xxxxxx - MSYS2 on Windows 11: <path_to_msys2>\tmp\epdfinfo_xxxxxx - macOS Ventura:/var/folders/xx/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/epdfinfo_xxxxxx Closes vedang#110
In d63a1e7, @JunyuanChen fixed a long standing compilation warning by replacing
tempnam
withmkstemp
inepdfinfo
. However, themkstemp
implementation does not work correctly on MS Windows (probably because the path template is "wrong" for Windows).I am reverting the commit and opening this issue to track the correct implementation of
mkstemp
(or equivalent) on MS Windows.It would also be great to add a test against Windows CI to open a PDF and check that the operation completes successfully.
The text was updated successfully, but these errors were encountered: