Implement `mkstemp` correctly for working against MSWindows

[vedang]

In d63a1e7d87f9b0a19209f2eeb170bcf64612aa2f, @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.

[JunyuanChen]

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]

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 https://github.com/vedang/pdf-tools/commit/d63a1e7d87f9b0a19209f2eeb170bcf64612aa2f )

--- 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 https://github.com/vedang/pdf-tools/issues/110#issuecomment-1121830575, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUUHVGZW73XHMPKOMYJGLVJHFMZANCNFSM5VP6AVCQ . You are receiving this because you authored the thread.Message ID: @.***>

[ShuguangSun]

It works.

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

[vedang]

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

[ShuguangSun]

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]

:( re-opening this ticket and reverting the commit https://github.com/vedang/pdf-tools/commit/37bbe861755bc60c7cc333359fee3e2a5d919c77

[ShuguangSun]

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

char template[] = "epdfinfoXXXXXX";

[vedang]

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 https://github.com/vedang/pdf-tools/issues/110#issuecomment-1124944263, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUUHSSWVCU5RERBP7CHN3VJT34XANCNFSM5VP6AVCQ . You are receiving this because you modified the open/close state.Message ID: @.***>

[vedang]

I've pushed the final version of this change to feature/replace-tempnam at https://github.com/vedang/pdf-tools/commit/7e0302ead166517253d1d8460d3ecba095970475 . 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:

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

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

[ShuguangSun]

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

char template[] = "epdfinfoXXXXXX";

[JunyuanChen]

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]

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.

[vedang]

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: https://github.com/vedang/pdf-tools/issues/110#issuecomment-1133793458

[ShuguangSun]

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]

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]

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

It doesn't work. The same error will be printed.