-
Notifications
You must be signed in to change notification settings - Fork 36
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
Comments
@alexvong243f can you look into this? |
In
|
Could this be a |
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
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... |
The other errors in that log look like:
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: |
Hold on, this construction is maybe suspicious to me:
Is there some problem about backslashes etc when you concatenate strings? Sounds familiar to me... |
Like does it help to use |
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. I don't know why this is a Cygwin Python and not a MinGW Python. |
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. |
@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: That may have been after octave 5,2.0 ? |
IIUC, that uses (an older version of) this package: 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 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. |
We can use the |
@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: 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 databasepacman -Sy install pythonpacman -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 |
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 |
There is also a new msys python - that would possibly handle names better? |
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 After that, we can use |
Unfortunately, using
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: The "proper" Unix-path that would correspond to the original file in the example above would be I'm not sure if this is a bug in 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. |
I wouldn't expect that it would behave much differently when it comes to paths. |
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:
IIUC, that corresponds to the test that is causing the error with the Octave for Windows installation. |
IIUC we can check if python is cygwin python using If we are indeed using cygwin python, perhaps we can just use cygwin mktemp to create temporary file, e.g.
Or we can get the path translation working. Does |
Maybe I was jumping to wrong conclusions regarding the path translation. 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:
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.
This is what I see for this command in the same Octave:
Am I doing something wrong? Edit: Slightly modified:
That is a bit irritating because that Python is not a MINGW64 application... Edit 2: Maybe, this instead?
Using Octave in MSYS2 (with native Python installed):
|
Fwiw, with the Octave in MSYS2 (and native Python), there are no unexpected failing tests:
|
I've somewhat lost the thread (or "path"? ;-) heh) here but no objection in principle: we have lots of "special cases" already... |
If I understand the current conclusions correctly, we could convert the paths that are passed to Python to make this work.
|
------- Original Message -------
On Tuesday, July 12th, 2022 at 5:49 AM, Markus Mützel ***@***.***> wrote:
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.
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.
… - If both compare true, convert the path with `cygpath -u`.
- Pass the converted path to the Python executable.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were assigned.
|
Not sure if this will work reliably in the future. Both |
I've kind lost the thread here: are we trying to fix |
If you talk about upstream Octave: There is probably not much that it could do differently. That leaves us with working around this in octsympy. I tried to outline what would be needed for that in the above comment. |
Thanks, then I think this urgent in that we need 3.0.1 with this fix before Octave 7.2.0 is out. |
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.
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.
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.
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.
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.
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.
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.
I think this can be closed. |
Add some comments explaining the situtation as best I know. See Issue #1182. Fixes [2]. [2] https://savannah.gnu.org/bugs/index.php?65735
from octave.discourse.group:
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
The text was updated successfully, but these errors were encountered: