Skip to content
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

tempfile related problems on MXE windows build #1182

Closed
cbm755 opened this issue Jul 7, 2022 · 30 comments
Closed

tempfile related problems on MXE windows build #1182

cbm755 opened this issue Jul 7, 2022 · 30 comments
Assignees
Milestone

Comments

@cbm755
Copy link
Collaborator

cbm755 commented Jul 7, 2022

from octave.discourse.group:

A couple of tests are failing in the CI with the nightly builds for Windows:
[freshly brewed Octave · gnu-octave/octave-buildbot@43f176e (github.com)](https://github.com/gnu-octave/octave-buildbot/runs/7238133932?check_suite_focus=true#step:13:15477)

Some of the errors seem to emit things like this:

  python: can't open file '/mingw64/share/octave/packages/symbolic-3.0.0/D:\a\_temp\oct-EEh6sa_octsympytmp.py': [Errno 2] No such file or directory

It looks like this could be two absolute paths that are concatenated. That is probably ok for most paths on a Linux file system. But it is failing on Windows.

Edit: That’s still with SymPy 1.5 in case that should matter.

updateI have trouble finding things later on discourse, not sure why, so I don't waste 5 mins in the future, here's the link https://octave.discourse.group/t/symbolic-3-0-0-released/2975/5

@cbm755 cbm755 added this to the 3.0.1 milestone Jul 7, 2022
@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 7, 2022

@alexvong243f can you look into this?

@alexvong243f
Copy link
Collaborator

In private/python_ipc_system.m, we currently do

    %% Generate a temp .py file then execute it with system()
    % can be useful for debugging, or if "python -c" fails for you
    if (isempty (tmpfilename))
      tmpfilename = [tempname() '_octsympytmp.py'];
      if (verbose)
        disp (['This session will use the temp file "' tmpfilename '"'])
      end
    end
    fd = fopen (tmpfilename, 'w');
    fprintf(fd, '# temporary autogenerated code\n\n');
    % we just added two more lines at the top
    info.prelines = info.prelines + 2;
    fputs(fd, bigs);
    fclose(fd);
    [status, out] = system ([pyexec ' ' tmpfilename]);

@alexvong243f
Copy link
Collaborator

Could this be a tempname() problem? How should we reproduce it?

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 8, 2022

I'm not sure :( I did run our test suite on a Windows machine (Octave 5.2.0) before releasing and I did not see this. Perhaps its something about cross-compiling? But I agree we're just using what tempname() gives us here, which is apparently

/mingw64/share/octave/packages/symbolic-3.0.0/D:\a\_temp\oct-EEh6sa_

That "D:" in the middle of a path seems invalid on Windows to me... @mmuetzel is it possible this is an upstream Octave / MXE builder problem...

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 8, 2022

The other errors in that log look like:

   Traceback (most recent call last):
    File "<string>", line 1, in <module>
  FileNotFoundError: [Errno 2] No such file or directory: 'D:\x07\\octave-buildbot\\octave-buildbot\\octave-2022-07-07-00-12-w64\\mingw64\\share\\octave\\packages\\symbolic-3.0.0\\private\\python_header.py'

Note the very different paths which look more reasonably and don't have a mixture of slashes and backslashes... unlike what tempname is giving us: /mingw64/share/octave/packages/symbolic-3.0.0/D:\a\_temp\oct-EEh6sa_

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 8, 2022

Hold on, this construction is maybe suspicious to me:

tmpfilename = [tempname() '_octsympytmp.py'];

Is there some problem about backslashes etc when you concatenate strings? Sounds familiar to me...

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 8, 2022

Like does it help to use strcat(tempname(), '_octsympytmp.py') here? I can't see any difference...

@cbm755 cbm755 changed the title new or continuing tempfile related problems tempfile related problems on MXE windows build Jul 8, 2022
@mmuetzel
Copy link
Member

mmuetzel commented Jul 8, 2022

Hmm... It looks like the Python that is included in Octave for Windows is for Cygwin. But the Octave program itself is for native MinGW.
By the looks of it, that Cygwin Python has issues when it gets passed a native Windows path. It expects a Cygwin path instead.

I don't know why this is a Cygwin Python and not a MinGW Python.
@lostbard: Would it be possible to include a MinGW Python in MXE Octave? Or do you have an idea how that could be solved otherwise?

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 9, 2022

For the record, I think the previous behaviour of Symbolic here was to create a temp file in the current working directory. Which is rude (see #1140). But that is presumably why previous versions did not hit this issue.

@lostbard
Copy link

lostbard commented Jul 9, 2022

@mmuetzel should already using msys2 python

in mxe: src/msys2-python.mk

At some point in the past we were using the embedded python rather than msys2, but have been with msys2 I believe for a while ?

in mxe:
changeset: 5493:dace3d372190
user: John Donoghue john.donoghue@ieee.org
date: Thu Jul 23 12:29:32 2020 -0400
summary: Use msys2 python3

That may have been after octave 5,2.0 ?

@mmuetzel
Copy link
Member

mmuetzel commented Jul 9, 2022

IIUC, that uses (an older version of) this package:
https://packages.msys2.org/base/python

The un-prefixed MSYS2 packages are basically Cygwin applications. (IIUC, the MSYS2 target is just a rechristened Cygwin with very minor modifications.) Those applications aren't really native to Windows. But they expect a Linux-like platform that is provided by Cygwin/MSYS2.

The native Windows applications are prefixed with mingw-w64 in MSYS2, and are distributed for different “environments" (e.g. MINGW32 or MINGW64). That would be one of these packages:
https://packages.msys2.org/base/mingw-w64-python

It looks like the Cygwin Python expects UNIX paths and cannot correctly handle the Windows path that it gets. That seems to be the cause for the issue here.

I don't know if it would be possible to just use the native Python from MSYS2 (dependency hell). I also don't know how hard it would be to cross-build a native Windows Python in MXE Octave.
But maybe there is also another solution I can't think of currently.
Do you have an idea how to solve this?

@alexvong243f
Copy link
Collaborator

We can use the cygpath program to convert windows path to cygwin path. cygpath should be provided by cygwin https://cygwin.com/cygwin-ug-net/cygpath.html

@lostbard
Copy link

lostbard commented Jul 9, 2022

@mmuetzel ok now im with you

It should be possible, but yeah - then requires getting all the dependancies correct.

Currently, https://packages.msys2.org/package/mingw-w64-x86_64-python lists dependancies as:
mingw-w64-x86_64-bzip2
mingw-w64-x86_64-expat
mingw-w64-x86_64-gcc-libs
mingw-w64-x86_64-libffi
mingw-w64-x86_64-mpdecimal
mingw-w64-x86_64-ncurses
mingw-w64-x86_64-openssl
mingw-w64-x86_64-sqlite3
mingw-w64-x86_64-tcl
mingw-w64-x86_64-tk
mingw-w64-x86_64-tzdata
mingw-w64-x86_64-xz
mingw-w64-x86_64-zlib

So would need them installed.

Possibly though, if we look at all the msys2 packages already installed, some may have mingw-64 equivalents and could also be used, perhaps allowing us to use more of them and less msys ones.

As a try of the mingw64 python, you could install it in octave-mxe after octave is installed via bash shell from the installed octave since there is enough msys2 utilities available in the install to do so:

update database

pacman -Sy

install python

pacman -S mingw-w64-x86_64-python

Im not sure if it will break anything else when it installs the dependancies, but worth a try perhaps

@lostbard
Copy link

lostbard commented Jul 9, 2022

actually gave it a try and it did conflict and not install so would need no deps (-dd) to see if will work, or force it to install and overwrite existing files

@lostbard
Copy link

lostbard commented Jul 9, 2022

There is also a new msys python - that would possibly handle names better?

@alexvong243f
Copy link
Collaborator

Regardless of which flavour of python is shipped with octave in windows, I think we still need code to check if the python we found accepts native path or cygwin posix path, as users can decide which python to use by specifying the PYTHON environment variable.

After that, we can use cygpath https://cygwin.com/cygwin-ug-net/cygpath.html to do path conversion if required.

@mmuetzel
Copy link
Member

mmuetzel commented Jul 10, 2022

Unfortunately, using cygpath wouldn't work in this particular case:

>> tmpfile = tempname()
tmpfile = C:\Users\Markus\AppData\Local\Temp\oct-rZDdea
>> system (['cygpath -u ', tmpfile])
/tmp/oct-rZDdea
ans = 0

For some reason, a path that points to a file at the native temporary directory seems to get translated to a path for a file in the MSYS2 temporary directory. Those are different locations in general. The latter has the following "Windows native" path for me: C:\Program Files\GNU Octave\Octave-7.1.0\tmp (or wherever Octave was installed).

The "proper" Unix-path that would correspond to the original file in the example above would be /C/Users/Markus/AppData/Local/Temp/oct-rZDdea. So, the file that cygpath returns wouldn't exist.

I'm not sure if this is a bug in cygpath or if this translation is done for some other compatibility(?) reason.

Even if there was an option to get the path translation that we'd need here, it would probably be hard to reliably detect automatically if the used Python is a Cygwin or a native Windows application.
Edit: And mixing native applications and Cygwin applications (like we currently do in MXE Octave) and expecting that path translations always happen automatically, might not be supported in general.

@mmuetzel
Copy link
Member

There is also a new msys python - that would possibly handle names better?

I wouldn't expect that it would behave much differently when it comes to paths.

@mmuetzel
Copy link
Member

To test if this would work correctly with a "native" Python, I installed Octave in the MINGW64 environment of MSYS2:

pacman -S mingw-w64-x86_64-octave mingw-w64-x86_64-python-pip
pip install sympy

And then tested the following in Octave:

octave:1> pkg install -forge symbolic
For information about changes from previous versions of the symbolic package, run 'news symbolic'.
octave:2> pkg load symbolic
octave:3> sympref('ipc', 'systmpfile');
Forcing systmpfile ipc: warning: this is for debugging
octave:4> syms x
Symbolic pkg v3.0.0: using slower system() communications with SymPy.
Warning: this will be *SLOW*.  Every round-trip involves executing a
new python process and many operations involve several round-trips.
This session will use the temp file "C:\msys64\tmp\oct-tOszqF_octsympytmp.py"
octave:5>

IIUC, that corresponds to the test that is causing the error with the Octave for Windows installation.
No errors here. The Windows native path seems to be handled correctly by the Windows native Python (as expected).

@alexvong243f
Copy link
Collaborator

alexvong243f commented Jul 10, 2022

IIUC we can check if python is cygwin python using python3 -c "import platform; platform.system()" python3 -c "import platform; print(platform.system())" as mentioned in https://stackoverflow.com/questions/42820914/check-if-running-in-windows-command-prompt-vs-cygwin-python

If we are indeed using cygwin python, perhaps we can just use cygwin mktemp to create temporary file, e.g.

[fin, fout, pid] = popen2 ('mktemp');

assert (fclose (fin) == 0);

tmpfilename = fgetl (fout)
assert (feof (fout));
assert (fclose (fout) == 0);

assert (waitpid (pid) >= 0);

Or we can get the path translation working. Does python3 "/tmp/oct-rZDdea" output nothing w/o error when using cygwin python?

@mmuetzel
Copy link
Member

mmuetzel commented Jul 10, 2022

Maybe I was jumping to wrong conclusions regarding the path translation.
For a test, I made the following change to python_ipc_system.m (in Octave for Windows from MXE Octave):

     info.prelines = info.prelines + 2;
     fputs(fd, bigs);
     fclose(fd);
-    [status, out] = system ([pyexec ' ' tmpfilename]);
+    [~, tmpfilename_u] = system (['cygpath -u ' tmpfilename]);
+    [status, out] = system ([pyexec ' ' tmpfilename_u]);
   end
 
   info.raw = out;
 

With it, I see the following:

>> sympref('ipc', 'systmpfile');

>> syms x

>>

So, that seems to be working (contrary to what I thought before). 🎉

If there is a way to reliably detect if Python is a Cygwin/MSYS2 or a native Windows application and you don't mind "polluting" your code with special cases, that might be a workable solution.
But it might need modifications in more places. With that change above applied, I still see failing test(s):

>> pkg load symbolic

>> test sympref

Traceback (most recent call last):
  File "<string>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\Octave\test\\octave-2022-07-07-09-04-w64 sympy-1.10.1\\octave-2022-07-07-09-04-w64\\mingw64\\share\\octave\\packages\\symbolic-3.0.0\\private\\python_header.py'
status = 1
out =
ind = [](0x0)
***** test
 % system should work on all system, but just runs sysoneline on windows
 sympref('ipc', 'system');
 syms x
!!!!! test failed
sysoneline ipc: system() call failed!
shared variables     sympref_orig =

      scalar structure containing the fields:

        ipc = default
        display = ascii
        digits = 32
        quiet = 0

>>

IIUC we can check if python is cygwin python using python3 -c "import platform; platform.system()"

This is what I see for this command in the same Octave:

>> system ('python3 -c "import platform; platform.system()"')
ans = 0
>> [a, b] = system ('python3 -c "import platform; platform.system()"')
a = 0
b =
>> [a, b] = system ('python -c "import platform; platform.system()"')
a = 0
b =

Am I doing something wrong?

Edit: Slightly modified:

>> [a, b] = system ('python -c "import platform; print(platform.system())"')
a = 0
b = MINGW64_NT-10.0-22000

That is a bit irritating because that Python is not a MINGW64 application...

Edit 2: Maybe, this instead?
Using Octave for Windows from MXE Octave:

>> [a, b] = system ('python -c "import os; print(os.name)"')
a = 0
b = posix

Using Octave in MSYS2 (with native Python installed):

octave:1> system('python3 -c "import os; print(os.name)"')
nt
ans = 0

@mmuetzel
Copy link
Member

Fwiw, with the Octave in MSYS2 (and native Python), there are no unexpected failing tests:

>> pkg test symbolic
[...]
Summary:

  PASS                             2402
  FAIL                                0
  XFAIL (expected failure)           31

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 10, 2022

and you don't mind "polluting" your code with special cases, that might be a workable solution.

I've somewhat lost the thread (or "path"? ;-) heh) here but no objection in principle: we have lots of "special cases" already...

@mmuetzel
Copy link
Member

If I understand the current conclusions correctly, we could convert the paths that are passed to Python to make this work.
IIUC, that would require these steps:

  • Identify where paths are passed as command line arguments to the Python executable.
  • Check whether Python is native or Cygwin on Windows. For this, we could probably use a combination of ispc and comparing the second output of system('python3 -c "import os; print(os.name)"') to posix. It might make sense to keep the result in a persisten variable to avoid spawning new Python processes if possible.
  • If both compare true, convert the path with cygpath -u.
  • Pass the converted path to the Python executable.

@alexvong243f
Copy link
Collaborator

alexvong243f commented Jul 12, 2022 via email

@mmuetzel
Copy link
Member

mmuetzel commented Jul 13, 2022

There's yet another way to do it (potentially simpler?). We can check if 'python3 -c "import os.path; print(os.path.sep)"' outputs the same separator as 'octave --eval "filesep()"'. If 'python3 -c "import os.path; print(os.path.sep)"' outputs '/' but 'octave --eval "filesep()"' outputs '', we know we are using cygwin/msys2 python.

Not sure if this will work reliably in the future. Both \ and / are valid file separators in Windows paths. Maybe Python or Octave might decide to default to / also on Windows at some point in the future.

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 18, 2022

I've kind lost the thread here: are we trying to fix tempname upstream or are we trying to workaround this in Symbolic?

@mmuetzel
Copy link
Member

If you talk about upstream Octave: There is probably not much that it could do differently.
If you talk about upstream MXE Octave: One possible solution might be to bundle a "native" Python instead of a Cygwin one. But building it seems to be rather difficult looking at the exorbitant number of patches that MSYS2 applies to build Python natively. That is probably out of scope for MXE Octave.

That leaves us with working around this in octsympy. I tried to outline what would be needed for that in the above comment.

@cbm755
Copy link
Collaborator Author

cbm755 commented Jul 18, 2022

Thanks, then I think this urgent in that we need 3.0.1 with this fix before Octave 7.2.0 is out.

alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jul 18, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/is_python_env_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jul 18, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/python_env_is_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jul 30, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/python_env_is_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jul 30, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/python_env_is_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jul 31, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/python_env_is_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jul 31, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/python_env_is_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Aug 2, 2022
This should fix some errors when Python is running in Cygwin-like
environment. But there could still be errors in other places.

See gnu-octave#1182.

* inst/private/cygpath.m: New function.
* inst/private/python_env_is_cygwin_like.m: New function.
* inst/private/python_ipc_system.m: Use them.
@cbm755
Copy link
Collaborator Author

cbm755 commented Aug 6, 2022

I think this can be closed.

@cbm755 cbm755 closed this as completed Aug 6, 2022
cbm755 added a commit that referenced this issue May 13, 2024
Add some comments explaining the situtation as best I know.
See Issue #1182.  Fixes [2].

[2] https://savannah.gnu.org/bugs/index.php?65735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants