robhagemans / pcbasic

PC-BASIC - A free, cross-platform emulator for the GW-BASIC family of interpreters
http://www.pc-basic.org
Other
396 stars 48 forks source link

Not always is needed ansipipe in windows #68

Closed Danitegue closed 6 years ago

Danitegue commented 6 years ago

Hi Rob. The new format of calling pcbasic works fine if I run it from a .bat file in windows. But with pycharm debugger in windows we have not the option to run pcbasic with the new format of the pcbasic launchers:

python -m pcbasic [+args] (Pycharm does not allow the -m option).

So I have no other option than calling in the old way:

python run.py [+args]

Also, when running from pycharm, we can't use ansipispe-launcher.exe, and we cant use the ansi console, we need to use the pygame or sdl2 consoles. So the pycharm launcher is more or less:

python run.py --interface=sdl2 [+args]

When running this with the last pcbasic versions, the winmain.py is automatically replacing the previous pycharm launcher line to this other launcher line:

ansipipe-launcher.exe python -m pcbasic --interface=sdl2 [+args]

which gives the following problems: -I explicitly don't want to use ansipipe-launcher when running from sourcecode in pycharm because it does not work with pycharm. -Even having the current dir correct, sys.path correct, and the ansipipe launcher path correct, the subprocess.call seems that cannot find the module pcbasic, when running with pycharm (I don't know why).

image

image

In any case, I think the source of the problem comes from the winmain function, in a way that it always does the automatic replacement of the launcher if it is detected a win32 platform, when, in my opinion, not in all the cases is needed to use ansipipe-launcher.exe when running in windows. (I would say that is only needed when running in windows+an ansi console is going to be used). (I can run a .bat launcher with sdl2 or pygame interfaces where no ansipipe launcher is needed)

So a solution can be: Do the automatic conversion of the launcher only if: -is detected in the arguments an ansi console selection AND the running plattform is win32.

which can be implemented by the change of the original winmain() function:

def winmain():
    """Windows console entry point."""
    if sys.platform != 'win32':
        from .main import main
        main()
    args = get_unicode_argv()
    startdir = os.path.join(os.path.dirname(os.path.realpath(__file__)))
    launcher = os.path.join(startdir, 'lib', 'ansipipe-launcher.exe')
    if os.path.isfile(launcher):
        # subprocess.call is not unicode-aware in Python 2
        # https://stackoverflow.com/questions/1910275/unicode-filenames-on-windows-with-python-subprocess-popen
        # see also https://gist.github.com/vaab/2ad7051fc193167f15f85ef573e54eb9 for a workaround
        # instead just encode in utf-8 and make our main entry point assume that on Windows
        # note that all this would be much easier if we didn't go though anispipe launcher
        args = [arg.encode('utf-8') for arg in args]
        # needed if called from repo source without pythonpath set
        # don't do os.chdir ans we need it for z: 
        sys.path.append(os.path.join(startdir, '..'))
        subprocess.call([launcher, sys.executable, '-m', 'pcbasic'] + args[1:])
    else:
        from .main import main
        main(*args)

to this proposal:

def winmain():
    """Windows console entry point."""
    args = get_unicode_argv()
    if (('--interface=ansi' in args) or not ('--interface' in args)) and sys.platform != 'win32':
        startdir = os.path.join(os.path.dirname(os.path.realpath(__file__)))
        launcher = os.path.join(startdir, 'lib', 'ansipipe-launcher.exe')
        if os.path.isfile(launcher):
            # subprocess.call is not unicode-aware in Python 2
            # https://stackoverflow.com/questions/1910275/unicode-filenames-on-windows-with-python-subprocess-popen
            # see also https://gist.github.com/vaab/2ad7051fc193167f15f85ef573e54eb9 for a workaround
            # instead just encode in utf-8 and make our main entry point assume that on Windows
            # note that all this would be much easier if we didn't go though anispipe launcher
            args = [arg.encode('utf-8') for arg in args]
            # needed if called from repo source without pythonpath set
            # don't do os.chdir ans we need it for z:
            sys.path.append(os.path.join(startdir, '..'))
            subprocess.call([launcher, sys.executable, '-m', 'pcbasic'] + args[1:])
        else:
            from .main import main
            main(*args[1:])
    else:
        from .main import main
        main(*args[1:])

I've tested it and now I can use both options:

Another solution could be leave it as it is, but adding the option to disable the automatic conversion, perhaps with an extra argument like --noansipipe, to avoid this automatic change of launcher when no needed.

robhagemans commented 6 years ago

Hi, I've been refactoring to get rid of the launcher (replacing it with a DLL that's loaded if needed). Have a look at the develop branch. Feel free to disable ansipipe-launcher altogether in the current setup, as you point out it works fine without it so long as you don't need the console.

robhagemans commented 6 years ago

Note also that the DLL approach in develop is probably going to be only temporary - the idea is to turn this into a proper Python extension module, so I can just do import without messing around with loading DLLs. But I'm still reading up on how to do that :)

(So please don't go and spend a lot of time changing your pcbasic-brewer setups to use this DLL)

The aim is to make it all a bit less fidgety and fragile on Windows - the launcher is causing way too many problems as it stands. Ideally people should be able to do pip install pcbasic and it just works.

Danitegue commented 6 years ago

Ok!. For now I would like to be aligned at least with your brewer branch, but with the improved optional logging that I have in pcbasic-brewer, since it is really useful for being able to determine if the brewer program is working as it should.

I mean the options I have added to write in the --logfile -the com port in/out messages (To develop the simulator, and also know if the communications with the instrument are working well, or why a communication was lost) -the shell outputs/answers (Porting win16/32 programs to win32/64 is not always straight forward). -the open/close of files (We can see what routines were running when an error happened) -when one determined variable has changed its value. (We can trace the value of a determined variable to see in what moment is changeing its value)

This is the unique difference btw pcbasic and my pcbasic_brewer just now.

This helps a lot to know if the things are not working as it should, and when is it happening. For example with these logs we have discovered that the sell command "copy file1+file2 destination" does not work fine in win10 32/64bits bits if the file1 doesnt exist, while in win98 16/32bits it created an empty file1 if it didnt exist to continue. To avoid that I'm now writting a python script to make pcbasic redirect all the shell commands:

instead of: --shell:cmd.exe use: --shell="python C:\PCBasic_Brewer_Repo\pcbasic_brewer\Brewerfunctions.py"

I will do another synchronization later, when everything works fine and pcbasic finally becomes in a python extension module as you propose.

robhagemans commented 6 years ago

Hi, yes, the incompatibility of various shells is one of the reasons I don't enable SHELL by default - there really is no way of telling what people have on their system (and I don't really want to deal with the support questions from people who don't understand this point..."SHELL is broken please fix"...). Also, cmd.exe is platform-dependent, it wouldn't work on Linux or Mac. Though wine cmd.exe is a reasonable standin, I've found.

I think a single-purpose shell stub like the Brewerfunctions.py you're proposing is a good idea, as it could remove the dependency on Windows versions and potentially make the brewer work the same on Windows and Mac as well.

Note that there's one thing that is not possible with the SHELL command on modern systems, and that is to change environment variables which are then used by BASIC programs. Changes made to the environment are not preserved when SHELL ends. As far as I understand it's not really possible to change this without some ugly hack - Windows, Unix and Mac simply work differently with the environment than MS-DOS. This is, for example, why SHELL "D:" can't change the current drive to D: in PC-BASIC. (I think the best "solution" to this would be to have a built-in shell inside PC-BASIC that implements an MS-DOS compatible prompt, but I don't think I'll be working on that soon. Your Brewerfunctions.py could be a start though...)

Note that I've been making some changes to SHELL in the develop branch but so long as it follows the same calling convention as cmd.exe (i.e. calling a DOS command with /c or /C) it should still work after those changes.