Change TMPPATH to the standard TMPDIR in configuration

[albinahlback]

Thanks to @orlitzky

[orlitzky]

Would it be feasible to query TMPDIR at runtime? The following python program,

from tempfile import NamedTemporaryFile
f = NamedTemporaryFile()
print(f.name)
f.close()

is able to use a different temporary location upon each execution:

$ python example.py 
/tmp/tmpm5vq1uo5
$ TMPDIR=/home/mjo python example.py 
/home/mjo/tmpilv5vtq6

I mainly use flint through sage, and sage through python, so in the rare event that someone wants to override TMPDIR in sage it would be nice if it moved flint's TMPDIR too.

[albinahlback]

We could check against the environment like we do in src/generic_files/test_helpers.c, but hopefully the file I/O could be completely removed from this function in the future.

[orlitzky]

That's a better idea, I didn't realize how isolated the temp path usage is. Thanks again.

[oscarbenjamin]

I'm wondering if this PR might be the cause of the problem I'm seeing with python_flint on Windows (built with mingw):
flintlib/python-flint#43 (comment)

The failure is:

Trying:
    fmpz(10**35 + 1).factor()
Expecting:
    [(11, 1), (9091, 1), (909091, 1), (4147571, 1), (265212793249617641, 1)]
Flint exception (General error):
    mkstemp failed

[orlitzky]

I'm wondering if this PR might be the cause of the problem I'm seeing with python_flint on Windows (built with mingw)

One guess: were you setting TMPPATH somewhere? If so, it should probably be updated to use TMPDIR instead.

Otherwise the easiest way to debug this is going to be to tweak https://github.com/flintlib/flint2/blob/ef89816abda221fc40cb4a960430cb570ef0f43c/src/qsieve/factor.c#L225 to output which error occurred. mkstemp() sets errno when it fails, so perror() should work as a quick hack.

[oscarbenjamin]

I don't have TMPPATH set locally but TMPDIR is set:

pc@NUCPC MINGW64 ~/src/flint/python-flint/test_dir/pr_flint3_artifact (pr_flint3)
$ echo $TMPPATH

(venv_ci_wheel)
pc@NUCPC MINGW64 ~/src/flint/python-flint/test_dir/pr_flint3_artifact (pr_flint3)
$ echo $TMPDIR
/tmp
(venv_ci_wheel)

I also see the failure in CI and I am not sure what environment variables are set there:

Run python test/dtest.py
  python test/dtest.py
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    pythonLocation: C:\hostedtoolcache\windows\Python\3.9.13\x64
    PKG_CONFIG_PATH: C:\hostedtoolcache\windows\Python\3.9.13\x64/lib/pkgconfig
    Python_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.9.13\x64
    Python2_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.9.13\x64
    Python3_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.9.13\x64
Flint exception (General error):
    mkstemp failed
Error: Process completed with exit code 1.

Otherwise the easiest way to debug this is going to be to tweak

I'll have to see if I reproduce this with a local build (rather than the CI build).

[orlitzky]

I don't have TMPPATH set locally but TMPDIR is set:

pc@NUCPC MINGW64 ~/src/flint/python-flint/test_dir/pr_flint3_artifact (pr_flint3)
$ echo $TMPPATH

(venv_ci_wheel)
pc@NUCPC MINGW64 ~/src/flint/python-flint/test_dir/pr_flint3_artifact (pr_flint3)
$ echo $TMPDIR
/tmp
(venv_ci_wheel)

I don't think these should be acting differently. The old behavior was to default to /tmp if TMPPATH is unset. The new behavior will use your TMPDIR, which is also /tmp...

[oscarbenjamin]

I don't think these should be acting differently.

Something has definitely changed :)

I changed the relevant code to:

    strcpy(qs_inf->fname, FLINT_TMPDIR "/siqsXXXXXX");
    printf("qsinf->fname= \"%s\"\n", qs_inf->fname);
    fd = mkstemp(qs_inf->fname);
    if (fd == -1) {
        perror("perror shows");
        flint_throw(FLINT_ERROR, "mkstemp failed\n");
    }

The output then is is:

Trying:
    fmpz(10**35 + 1).factor()
Expecting:
    [(11, 1), (9091, 1), (909091, 1), (4147571, 1), (265212793249617641, 1)]
qsinf->fname= "/tmp/siqsXXXXXX"
perror shows: No such file or directory
Flint exception (General error):
    mkstemp failed

I tried to reproduce that in plain C without flint etc:

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
    int fd = mkstemp("/tmp/siqsXXXXXX");
    return 0;
}

That gives a segfault though (I must be doing something stupid here but I'm too tired to see it!):

$ gcc test.c
$ ./a.exe
Segmentation fault

[oscarbenjamin]

too tired to see it

And in the morning it becomes obvious. The argument to mkstemp is
supposed to be a mutable string:

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
    char fname[] = "/tmp/siqsXXXXXX";
    int fd = mkstemp(fname);
    printf("fd = %d\n", fd);
    perror("perror shows");
    return 0;
}

This program runs fine on Linux and shows:

$ gcc test.c
$ ./a.out
fd = 3
perror shows: Success

However it fails with "no such file or directory" when compiled with mingw64 and run under Windows:

pc@NUCPC MINGW64 /c/Users/pc/src/flint/python-flint/test_dir/test_mkstemp
$ gcc --version
gcc.exe (Rev4, Built by MSYS2 project) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


pc@NUCPC MINGW64 /c/Users/pc/src/flint/python-flint/test_dir/test_mkstemp
$ cat test.c


#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
    char fname[] = "/tmp/siqsXXXXXX";
    int fd = mkstemp(fname);
    printf("fd = %d\n", fd);
    perror("perror shows");
    return 0;
}

pc@NUCPC MINGW64 /c/Users/pc/src/flint/python-flint/test_dir/test_mkstemp
$ gcc test.c

pc@NUCPC MINGW64 /c/Users/pc/src/flint/python-flint/test_dir/test_mkstemp
$ ./a.exe
fd = -1
perror shows: No such file or directory

[oscarbenjamin]

Comparing with Flint 2.9.0 it seems that mkstemp was not even called. This PR probably did not originate the failure that I am seeing then and it goes back to the prior gh-1201 which introduced mkstemp.

[oscarbenjamin]

I'm not sure how this is supposed to work with mingw. in cmd.exe the TMP env var is set correctly:

C:\Users\pc>echo %TMP%
C:\Users\pc\AppData\Local\Temp

In the mingw shell it shows differently:

$ echo $TMP
/tmp

However mingw translates this to the same directory:

pc@NUCPC MINGW64 ~/src/flint/python-flint (pr_flint3)
$ echo contents > /tmp/stuff.txt

pc@NUCPC MINGW64 ~/src/flint/python-flint (pr_flint3)
$ cat ~/AppData/Local/Temp/stuff.txt
contents

If I change the path in the test program above explicitly to char fname[] = "C:\\Users\\pc\\AppData\\Local\\Temp\\siqsXXXXXX" then it works. Supposedly /tmp should map through to that somehow though.

I am not sure exactly when and where this translation between unix paths and Windows paths happens. At runtime the flint code sees $TMP="/tmp" even if I run it from outside of mingw in cmd.exe where we actually have %TMP%=C:\Users\pc\AppData\Local\Temp.

Presumably then it is the job of that mingw translation layer somewhere to make mkstemp work with unix paths but I guess that is not happening in this case?

[oscarbenjamin]

I'm not sure but this seems like a mingw bug. I'm just wondering now what flint/python_flint should do about that.

Taking a step back it seems somewhat surprising that fmpz_factor creates files in the filesystem at all. I can understand why that happens but in general I would expect that an API that does this would provide some way to control that like either preventing the creation of files, controlling where they go, perhaps limiting the size of the files etc. If python_flint could control that somehow then I am sure that it would be more reliable to determine this path from Python:

>>> import tempfile
>>> tempfile.gettempdir()
'C:\\Users\\pc\\AppData\\Local\\Temp'

There are a few functions that indirectly call through to qsieve_factor so a global configuration seems to make more sense rather than propagating configuration options through the signatures of many functions. There is the FLINT_TMPDIR configuration variable but IIUC it can only be set at build time. Perhaps if that could also be used as a runtime environment variable then python_flint could set that from Python at import time before calling any function like fmpz_factor.

[oscarbenjamin]

At runtime the flint code sees $TMP="/tmp" even if I run it from outside of mingw in cmd.exe where we actually have %TMP%=C:\Users\pc\AppData\Local\Temp

Actually this is not true. The problem is just that "/tmp" is baked into the binary.

There is the FLINT_TMPDIR configuration variable but IIUC it can only be set at build time. Perhaps if that could also be used as a runtime environment variable then python_flint could set that from Python at import time before calling any function like fmpz_factor.

Or perhaps just the TMPDIR environment variable should be consulted at runtime rather than at build time. In the python_flint context the builds are intended for redistribution so it does not make sense to fix any value for this at build time.

Still though being able to set this explicitly from the Python side would be better in python_flint's case.

[orlitzky]

Actually this is not true. The problem is just that "/tmp" is baked into the binary.

So the translation happens in the environment, and not in the call to mkstemp()? That makes sense, or at least explains why it's failing.

Taking a step back it seems somewhat surprising that fmpz_factor creates files in the filesystem at all... Or perhaps just the TMPDIR environment variable should be consulted at runtime rather than at build time. In the python_flint context the builds are intended for redistribution so it does not make sense to fix any value for this at build time.

Using $TMPDIR at runtime should be OK. That's typically how it works. It would be a breaking change, but a very minor one. The few users who need to change TMPDIR (like flint's CI) would just have to make sure it's exported at runtime and not only at build time.

But, yes, this one function is the only place that uses the temporary directory, and it can probably be eliminated. That actually already came up earlier in the discussion: #1334 (comment) #1334 (comment)

[oscarbenjamin]

So the translation happens in the environment, and not in the call to mkstemp()?

I think so. I find it hard to be sure about anything with MinGW but I think the idea is that although its shell messes around with things to emulate a unix-like environment the binaries produced by gcc are still supposed to be ordinary Windows binaries. Within the binary itself all paths should be Windows paths and so /tmp does not exist in the filesystem.

If I am right about that then this is not a mingw bug: the bug is that Flint hardcodes /tmp as the temporary files directory. Technically it is not hardcoded since you can set it at build time but on Windows it is not possible to know what the tmpdir should be until runtime because even the default path includes the username.

The non-MinGW Windows path here uses tmpnam which has the advantage that there is no need to try to guess the path at all although the Linux manpage for tmpnam says:
"Never use these functions. Use mkstemp(3) or tmpfile(3) instead."
https://man7.org/linux/man-pages/man3/tmpnam.3.html
It looks like tmpfile also has the advantage of not needing to guess any paths so maybe that is the best option for all platforms. I haven't used it before though so I can't be sure.

It looks like tmpfile is C89 and MSVC has it although describes it as deprecated in favour of tmpfile_s that apparently has better security (not sure if that is really relevant here).

The few users who need to change TMPDIR (like flint's CI)

Presumably tmpfile would fail in that case as well?

The Linux and OSX CI for python_flint pass without having configured anything for FLINT_TMPDIR which suggests that the default Flint build works in GitHub Actions for those platforms. It is only the Windows (MinGW) CI that fails because of the problem with mkstemp:
https://github.com/fredrik-johansson/python-flint/actions/runs/5761150407/job/15618457139?pr=43

I guess if Flint CI uses FLINT_TMPDIR to change the directory used then that explains why this bug is not seen in Flint's own MinGW CI job.

But, yes, this one function is the only place that uses the temporary directory, and it can probably be eliminated.

Ultimately that would be best. For now though this leaves python_flint in a bit of a bind: without some change in Flint there is nothing that python_flint can do to generate a wheel whose fmpz.factor() function wouldn't crash at runtime on Windows. The crash is not even an exception that can be caught as it kills Python completely.

What python_flint needs is one of:

1. Flint chooses valid files automatically at runtime.
2. Flint provides a way to configure what path is used at runtime.
3. Flint does not create any files at all.

If the ultimate plan is 3. then probably 2. is not a good temporary solution. Perhaps then the proper fix is 1. using something like tmpfile until 3. is implemented in future.

[oscarbenjamin]

I guess if Flint CI uses FLINT_TMPDIR to change the directory used then that explains why this bug is not seen in Flint's own MinGW CI job.

Actually it is only the MinGW CI job that uses TMPDIR:
https://github.com/flintlib/flint2/blob/ef89816abda221fc40cb4a960430cb570ef0f43c/.github/workflows/CI.yml#L473-L475
The comment there is incorrect: there is no /tmp directory on Windows. It looks like TMPDIR is only there to avoid the bug that I have been investigating.

[orlitzky]

One thing I still don't understand is, how did the behavior change? Even before #1201, /tmp was hard-coded. In any case, I am mainly chiming in to say please open a new issue so it isn't forgotten. I don't think there's a good reason for using a temporary file in the first place except to avoid some memory management, so eliminating the file would make everybody happy.

[oscarbenjamin]

One thing I still don't understand is, how did the behavior change?

I presume you have read the lengthy explanation in gh-1416.

Even before #1201, /tmp was hard-coded.

That's not true unless I've misunderstood something. Before gh-1201 the current working directory was used (as explained in that PR). The prior code was:

#if defined (__WIN32) && !defined(__CYGWIN__)
    srand((int) GetCurrentProcessId());
#else
    srand((int) getpid());
#endif
    nchars = sprintf(qs_inf->fname, "%d", (int) rand());
    strcat(qs_inf->fname + nchars, "siqs.dat");

    qs_inf->siqs = fopen(qs_inf->fname, "w");

So the pid is used to seed rand which is used to generate a filename like 12345siqs.dat. No directory is given so cwd is used implicitly. I think I can remember seeing these files left over sometimes after running the python-flint test suite.

As of gh-1416 the appropriate temporary directory is used on Windows or for other platforms it is hard-coded to /tmp. I don't know if there are other platforms where /tmp would not be writable (or is not actually used for temporary files) but at least it should be good for OSX and popular Linuxes. The current working directory is also not guaranteed to be writable so the Flint 2.9.0 code would fail in some situations as well.

This is not really my expertise but I think that /tmp is part of the POSIX standards:

The /tmp directory is retained in POSIX.1-2017 to accommodate historical applications that assume its availability. Implementations are encouraged to provide suitable directory names in the environment variable TMPDIR and applications are encouraged to use the contents of TMPDIR for creating temporary files.

https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap10.html

That suggests that gh-1416 should be fine and that the only way to potentially improve it is to use the environment variable TMPDIR but presumably it is supposed to be checked at run time rather than build time. Given cross-compiling, multi-user systems, etc checking TMPDIR at build time might be less reliable than hard-coding /tmp.

[orlitzky]

Even before #1201, /tmp was hard-coded.

That's not true unless I've misunderstood something.

You're right, I misread the diff.

That suggests that gh-1416 should be fine and that the only way to potentially improve it is to use the environment variable TMPDIR but presumably it is supposed to be checked at run time rather than build time. Given cross-compiling, multi-user systems, etc checking TMPDIR at build time might be less reliable than hard-coding /tmp.

Agreed as far as temporary file handling goes, but doing it in memory would eliminate the platform-specific #ifdefs.

[oscarbenjamin]

but doing it in memory would eliminate the platform-specific #ifdefs.

Yes, of course that would be better. Removing the use of files seems to have been suggested a few times and I'm sure several people have looked at it and thought about it. I did consider changing that myself but then when I actually looked at the code I immediately decided to go for a quicker fix.