Implement `mkstemp` correctly for working against MSWindows #110

vedang · 2022-05-10T01:09:43Z

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 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:

@JunyuanChen

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

JunyuanChen · 2022-05-10T02:51:45Z

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)

--- 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));

vedang · 2022-05-10T12:36:02Z

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: ***@***.***>

ShuguangSun · 2022-05-11T03:15:50Z

It works.

FYI. In windows 10, the temp file goes to "c:/Users/username/AppData/Local/Temp/pdf-tools-"xxx as expected.

vedang · 2022-05-11T15:04:54Z

Thank you for the confirmation! I will make the change today!

ShuguangSun · 2022-05-12T01:07:26Z

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".

vedang · 2022-05-12T01:44:58Z

:( re-opening this ticket and reverting the commit 37bbe86

@ShuguangSun

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

ShuguangSun · 2022-05-12T12:40:31Z

At Windows 10, It seems no need of the path to temp dir.

char template[] = "epdfinfoXXXXXX";

vedang · 2022-05-13T12:04:59Z

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: ***@***.***>

@ShuguangSun

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

@ShuguangSun

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

vedang · 2022-05-22T01:03:02Z

I've pushed the final version of this change to feature/replace-tempnam at 7e0302e . However, I cannot merge it in unless it is tested and working on multiple operating systems.

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:

Clone pdf-tools from the feature/replace-tempnam branch.
Compile epdfinfo by running make in the cloned directory.
If step 2 succeeds, find and replace the epdfinfo in your .emacs.d with the newly compiled one.
Restart your Emacs session
Open any PDF file. It should render correctly. Our goal is to test that all annotations are working correctly.
Annotation operations are described here: https://pdftools.wiki/243b3843 <- Create annotations of all types.
Save the file. Then kill the buffer and reload the file from disk.
Check if all the annotations are preserved properly.

If all these steps work, then the change is working correctly on your operation system.

ShuguangSun · 2022-05-22T08:09:57Z

At Windows 10, It seems no need of the path to temp dir.

char template[] = "epdfinfoXXXXXX";

JunyuanChen · 2022-05-23T02:38:50Z

If you run printf "#include <stdio.h>\nP_tmpdir" | gcc -E - | tail -n1 in the shell, what do you get? On Linux with glibc 2.35 it tells me "/tmp", so P_tmpdir "epdfinfo_XXXXXX" will expand to "/tmpepdfinfo_XXXXXX" which looks like it won’t work.

ShuguangSun · 2022-05-23T09:27:42Z

If you run printf "#include <stdio.h>\nP_tmpdir" | gcc -E - | tail -n1 in the shell, what do you get? On Linux with glibc 2.35 it tells me "/tmp", so P_tmpdir "epdfinfo_XXXXXX" will expand to "/tmpepdfinfo_XXXXXX" which looks like it won’t work.

It print "\\" on MINGW64 in a Windows box.

@ShuguangSun

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

@ShuguangSun

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

vedang · 2022-05-24T02:46:56Z

Hi @ShuguangSun and others on this thread,

I've pushed the following change to the feature/replace-tempnam branch:

@@ -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'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)

ShuguangSun · 2022-05-24T06:11:25Z

It does'nt work on windows. As it pointed above P_tmpdir is "\\" (\ I think) on windows, P_tmpdir MKTEMP_SEPARATOR might be "\\\\" (\\).

It works well if it changes to char template[] = "epdfinfoXXXXXX";. Might it go to c:\Users\<username>\Appata\Local\Temp directly?

vedang · 2022-05-24T13:37:05Z

It does'nt work on windows. As it pointed above P_tmpdir is "\\" (\ I think) on windows, P_tmpdir MKTEMP_SEPARATOR might be "\\\\" (\\).

Does it work with char template[] = P_tmpdir "epdfinfoXXXXXX"; on Windows?

It works well if it changes to char template[] = "epdfinfoXXXXXX";. Might it go to c:\Users\<username>\Appata\Local\Temp directly?

I am not sure where it goes. This is also why I'm not keen on merging char template[] = "epdfinfoXXXXXX"; as the actual solution (even if I manage to restrict it just to Windows and use P_tmpdir on other platforms)

ShuguangSun · 2022-05-25T00:46:35Z

Does it work with char template[] = P_tmpdir "epdfinfoXXXXXX"; on Windows?

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. 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

vedang added the MSWindows Issues related to running pdf-tools on Windows label May 10, 2022

vedang added the help wanted Extra attention is needed label May 10, 2022

vedang closed this as completed in 37bbe86 May 11, 2022

vedang reopened this May 12, 2022

vedang added the compatibility and installation Keeping up with changes in the rest of the environment label May 17, 2022

vedang added macOS Issues related to running pdf-tools on macOS Linux Needs testing / work on Linux distributions. labels May 22, 2022

ywwry66 linked a pull request Sep 11, 2023 that will close this issue

Use mkstemp instead of tempnam #235

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `mkstemp` correctly for working against MSWindows #110

Implement `mkstemp` correctly for working against MSWindows #110

vedang commented May 10, 2022

JunyuanChen commented May 10, 2022

vedang commented May 10, 2022 via email

ShuguangSun commented May 11, 2022

vedang commented May 11, 2022

ShuguangSun commented May 12, 2022 •

edited

vedang commented May 12, 2022

ShuguangSun commented May 12, 2022

vedang commented May 13, 2022 via email

vedang commented May 22, 2022

ShuguangSun commented May 22, 2022

JunyuanChen commented May 23, 2022

ShuguangSun commented May 23, 2022 •

edited

vedang commented May 24, 2022 •

edited

ShuguangSun commented May 24, 2022 •

edited

vedang commented May 24, 2022 •

edited

ShuguangSun commented May 25, 2022

Implement mkstemp correctly for working against MSWindows #110

Implement mkstemp correctly for working against MSWindows #110

Comments

vedang commented May 10, 2022

JunyuanChen commented May 10, 2022

vedang commented May 10, 2022 via email

ShuguangSun commented May 11, 2022

vedang commented May 11, 2022

ShuguangSun commented May 12, 2022 • edited

vedang commented May 12, 2022

ShuguangSun commented May 12, 2022

vedang commented May 13, 2022 via email

vedang commented May 22, 2022

ShuguangSun commented May 22, 2022

JunyuanChen commented May 23, 2022

ShuguangSun commented May 23, 2022 • edited

vedang commented May 24, 2022 • edited

ShuguangSun commented May 24, 2022 • edited

vedang commented May 24, 2022 • edited

ShuguangSun commented May 25, 2022

Implement `mkstemp` correctly for working against MSWindows #110

Implement `mkstemp` correctly for working against MSWindows #110

ShuguangSun commented May 12, 2022 •

edited

ShuguangSun commented May 23, 2022 •

edited

vedang commented May 24, 2022 •

edited

ShuguangSun commented May 24, 2022 •

edited

vedang commented May 24, 2022 •

edited