Skip to content

Perform the executable validation along with the version check in _create_repl() #241

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tikuma-lsuhsc
Copy link

@tikuma-lsuhsc tikuma-lsuhsc commented Aug 20, 2024

This PR is my solution to an octave-path issue I just run into running oct2py.

In Windows, my default Octave installation path has spaces, and it is not boding well with how the subprocess is spawned when used with the OCTAVE_EXECUTABLE environment. I tried to circumvent this issue by using the DOS path which can be generated easily with a Windows batch file (an example here) but it produces an all-caps space-free path, which fails the executable finding mechanism.

There are two approaches to alleviate this issue: (a) to allow spaces in the path and (b) to allow octave path to be not containing case-sensitive 'octave' string. (There is another of manually fixing the capitalization but that shouldn't be needed.) Based on my code review, it appears that the former must be addressed by the metakernel package while the latter can be addressed by this package. And a proposed solution is this PR.

The Octave name check is currently done in OctaveEngine._get_executable() and there is a version checking in OctaveEngine._create_repl() if the name checking fails (or if 'snap' is used, which I'm not familiar with). It seems sensible to remove the octave name check in the path and perform the executable check along with the version check because the executable file can be renamed to whatever by user or an alternate path like in my case with the DOS path.

I tested the PR with a simple test:

from octave_kernel.kernel import OctaveEngine
engine = OctaveEngine()

P.S., I actually think allowing the spaces in the path is the correct fix, and I'll see if metakernel maintainer agrees for my proposed change, I'll get back here later for another PR if acceptable. if a solution for it is desirable, I will create a PR on metakernel repo later to enable the fix. (edit: I didn't realize both are under the same org)

@tikuma-lsuhsc tikuma-lsuhsc reopened this Aug 20, 2024
@tikuma-lsuhsc
Copy link
Author

One more thing. I assumed that the --version string always starts with "GNU Octave" in the earlier versions (I've only tested on the latest, v9.2). This needs to be checked against earlier versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant