bpo-14916: use specified tokenizer fd for file input

[pmp-p]

PyRun_InteractiveOne*() functions allow to explicitily set fd instead of stdin.
but stdin was hardcoded in readline call.

This patch does not fix target file for prompt unlike original bpo one : prompt fd is unrelated to tokenizer source which could be read only.

Also actual documentation say "prompt the user" so one would expect prompt to go on stdout not a file for both PyRun_InteractiveOne*() and PyRun_InteractiveLoop*().

EDIT/ to be clear this is a 🔴type-bugfix🔴

https://bugs.python.org/issue14916

[pablogsal]

Hi, @pmp-p for the contribution. I will try to review the issue and the PR in the following fsyd. Could you please add some test that exercises this meanwhile?

[pablogsal]

Also, please add a NEWS entry describing the fix

[ta0kira]

I think that "interactive" implies that fp should be R/W, or the prompt writes should just be allowed to fail internally if the file is opened R/O.

I'm still not sure if the blocking issue exists (see https://bugs.python.org/issue14916#msg184410), but it seems like the interactive execution should really use separate input and output descriptors, with a target use-case being a PTY descriptor that's been duplicated. Since that might require a lot of changes, I think a sufficient compromise would be to use fp for both input and output.

(BTW, I have no say in the approval of this PR; I'm just the one who originally created https://bugs.python.org/issue14916.)

[pmp-p]

@pablogsal , here's the test i use :

#!/bin/sh

PYVER=${PYVER:-3.8}

echo "testing [bpo-14916](https://bugs.python.org/issue14916) for python${PYVER}"


cat <<END > test-[bpo-14916](https://bugs.python.org/issue14916).c
#include <stdio.h>
#include <stdlib.h>

#define PY_SSIZE_T_CLEAN
#include <Python.h>

#define MAX 1024
char buf[MAX];

#define TEST_CODE "file_io"

int
main(int argc, char *argv[])
{
    wchar_t *program = Py_DecodeLocale(argv[0], NULL);
    if (program == NULL) {
        fprintf(stderr, "Fatal error: cannot decode argv[0]\n");
        exit(1);
    }
    Py_SetProgramName(program);  /* optional but recommended */
    Py_Initialize();

    FILE *one_file = fopen(TEST_CODE,"w+");

    setenv("BPO14916","FOUND!",1);

    fputs("__import__('os').environ['BPO14916']='not present'\n", one_file);

    fflush(one_file);
    fclose(one_file);



    one_file = fopen(TEST_CODE, "r");
    int line =  0;

    if (one_file) {
        while( fgets(&buf[0], MAX, one_file) ) {
            fprintf( stderr, "%d: %s",++line, buf );
        }


        rewind(one_file);
        int res = PyRun_InteractiveOne( one_file, TEST_CODE);

       fclose(one_file);
    } else {
        fprintf( stderr, "IO error\n");
    }


    printf("\nStatus of bug 14916 : %s\n\n", getenv("BPO14916") );

    if (Py_FinalizeEx() < 0) {
        exit(120);
    }
    PyMem_RawFree(program);
    return 0;
}

END

gcc -o test-[bpo-14916](https://bugs.python.org/issue14916) test-[bpo-14916](https://bugs.python.org/issue14916).c -I/usr/local/include/python${PYVER} -L/usr/local/lib -lpython${PYVER} -lpthread -ldl -lm -lutil

echo i_should_not_be_parsed | ./test-[bpo-14916](https://bugs.python.org/issue14916)

[pmp-p]

attaching file because it seems the bevedere-bot parser will modify code blocks it should not
test.txt

@ta0kira, i'm just trying here to fix the obvious parser bug, my use case does not use PTY so i don't know the side effects you may have encountered.

I concur, actual repl adapters are weak for some use cases, ideally there should be one with three fd in ( line based) / out / err + an event source ( character based ) but afaik the asyncio shell from 3.8+ is tending toward that.

[pmp-p]

@pablogsal sorry to bother pinging you, but that bug is getting annoying for a repl based testuite i'd like to demonstrate for zipimport, android and web. Do you need another kind of test instead of C one ?

[pmp-p]

As @JulienPalard pointed it out, I don't think there should be an implicit fallback to stdin but it was in original PR. It would be nice to have any reviewer's advice.

[JulienPalard]

LGTM (but the space).

Tested on master. It make sense to me because the fd is given to PyParser_ASTFromFileObject up to PyTokenizer_FromFile which stores it in tok->fp.

Also on line 835 there's a similar condition : read from the file if tok->fp is not null, else read from stdin.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[pmp-p]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@JulienPalard: please review the changes made to this pull request.

[pablogsal]

This is still missing an automated test. Please, include one in the test suite.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[pmp-p]

@pablogsal sorry, i don't know how to test C-API from cpython testsuite on official platforms, i'm just a embedding user of it. Also the only time i had to spend was on the automated C linux above.