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

Argparse for cython #3001

Merged
merged 16 commits into from Aug 4, 2019
Merged

Argparse for cython #3001

merged 16 commits into from Aug 4, 2019

Conversation

realead
Copy link
Contributor

@realead realead commented Jun 13, 2019

This is a continuation to #2952: replacing a self-made command line parser for cython.py with argparse, which is already used by cythonize and (kind of) by IPython-magic.

Using only one library for argument parsing will make it easier to add new options to cython/cythonize/IPython-magic and/or syncronize/consolidate them all.

Apart of a slightly different help message there should be no behavioral changes.

@realead
Copy link
Contributor Author

realead commented Jun 19, 2019

It is not easy to map --embed=<method_name> onto argparse-concepts. The current unpretty workaround is https://github.com/cython/cython/pull/3001/commits/7a2b616878dd244f09e9f89979b584ee674934c0 Don't know how widely --embed=some_method_name is used and whether a breaking change is thinkable in order to avoid this special handling.

@realead
Copy link
Contributor Author

realead commented Jun 19, 2019

Old help message:

Cython (http://cython.org) is a compiler for code written in the
Cython language.  Cython is based on Pyrex by Greg Ewing.

Usage: cython [options] sourcefile.{pyx,py} ...

Options:
  -V, --version                  Display version number of cython compiler
  -l, --create-listing           Write error messages to a listing file
  -I, --include-dir <directory>  Search for include files in named directory
                                 (multiple include directories are allowed).
  -o, --output-file <filename>   Specify name of generated C file
  -t, --timestamps               Only compile newer source files
  -f, --force                    Compile all source files (overrides implied -t)
  -v, --verbose                  Be verbose, print file names on multiple compilation
  -p, --embed-positions          If specified, the positions in Cython files of each
                                 function definition is embedded in its docstring.
  --cleanup <level>              Release interned objects on python exit, for memory debugging.
                                 Level indicates aggressiveness, default 0 releases nothing.
  -w, --working <directory>      Sets the working directory for Cython (the directory modules
                                 are searched from)
  --gdb                          Output debug information for cygdb
  --gdb-outdir <directory>       Specify gdb debug information output directory. Implies --gdb.

  -D, --no-docstrings            Strip docstrings from the compiled module.
  -a, --annotate                 Produce a colorized HTML version of the source.
  --annotate-coverage <cov.xml>  Annotate and include coverage information from cov.xml.
  --line-directives              Produce #line directives pointing to the .pyx source
  --cplus                        Output a C++ rather than C file.
  --embed[=<method_name>]        Generate a main() function that embeds the Python interpreter.
  -2                             Compile based on Python-2 syntax and code semantics.
  -3                             Compile based on Python-3 syntax and code semantics.
  --3str                         Compile based on Python-3 syntax and code semantics without
                                 assuming unicode by default for string literals under Python 2.
  --lenient                      Change some compile time errors to runtime errors to
                                 improve Python compatibility
  --capi-reexport-cincludes      Add cincluded headers to any auto-generated header files.
  --fast-fail                    Abort the compilation on the first error
  --warning-errors, -Werror      Make all warnings into errors
  --warning-extra, -Wextra       Enable extra warnings
  -X, --directive <name>=<value>[,<name=value,...] Overrides a compiler directive
  -E, --compile-time-env name=value[,<name=value,...] Provides compile time env like DEF would do.

new help message:

usage: cython.py [-h] [-V] [-l] [-I INCLUDE_PATH] [-o OUTPUT_FILE] [-t] [-f]
                 [-v] [-p] [--cleanup GENERATE_CLEANUP_CODE] [-w WORKING_PATH]
                 [--gdb] [--gdb-outdir GDB_OUTDIR] [-D] [-a]
                 [--annotate-fullc]
                 [--annotate-coverage ANNOTATE_COVERAGE_XML]
                 [--line-directives] [-+] [--embed] [-2] [-3] [--3str]
                 [--lenient] [--capi-reexport-cincludes] [--fast-fail]
                 [-Werror] [-Wextra] [-X NAME=VALUE,...] [-E NAME=VALUE,...]
                 [sources [sources ...]]

Cython (https://cython.org/) is a compiler for code written in the Cython
language. Cython is based on Pyrex by Greg Ewing.

positional arguments:
  sources

optional arguments:
  -h, --help            show this help message and exit
  -V, --version         Display version number of cython compiler
  -l, --create-listing  Write error messages to a listing file
  -I INCLUDE_PATH, --include-dir INCLUDE_PATH
                        Search for include files in named directory (multiple
                        include directories are allowed).
  -o OUTPUT_FILE, --output-file OUTPUT_FILE
                        Specify name of generated C file
  -t, --timestamps      Only compile newer source files
  -f, --force           Compile all source files (overrides implied -t)
  -v, --verbose         Be verbose, print file names on multiple compilation
  -p, --embed-positions
                        If specified, the positions in Cython files of each
                        function definition is embedded in its docstring.
  --cleanup GENERATE_CLEANUP_CODE
                        Release interned objects on python exit, for memory
                        debugging. Level indicates aggressiveness, default 0
                        releases nothing.
  -w WORKING_PATH, --working WORKING_PATH
                        Sets the working directory for Cython (the directory
                        modules are searched from)
  --gdb                 Output debug information for cygdb
  --gdb-outdir GDB_OUTDIR
                        Specify gdb debug information output directory.
                        Implies --gdb.
  -D, --no-docstrings   Strip docstrings from the compiled module.
  -a, --annotate        Produce a colorized HTML version of the source.
  --annotate-fullc      Produce a colorized HTML version of the source which
                        includes entire generated C/C++-code.
  --annotate-coverage ANNOTATE_COVERAGE_XML
                        Annotate and include coverage information from
                        cov.xml.
  --line-directives     Produce #line directives pointing to the .pyx source
  -+, --cplus           Output a C++ rather than C file.
  --embed               Generate a main() function that embeds the Python
                        interpreter. Pass --embed=<method_name> for a name
                        other than main().
  -2                    Compile based on Python-2 syntax and code semantics.
  -3                    Compile based on Python-3 syntax and code semantics.
  --3str                Compile based on Python-3 syntax and code semantics
                        without assuming unicode by default for string
                        literals under Python 2.
  --lenient             Change some compile time errors to runtime errors to
                        improve Python compatibility
  --capi-reexport-cincludes
                        Add cincluded headers to any auto-generated header
                        files.
  --fast-fail           Abort the compilation on the first error
  -Werror, --warning-errors
                        Make all warnings into errors
  -Wextra, --warning-extra
                        Enable extra warnings
  -X NAME=VALUE,..., --directive NAME=VALUE,...
                        Overrides a compiler directive
  -E NAME=VALUE,..., --compile-time-env NAME=VALUE,...
                        Provides compile time env like DEF would do.

@realead
Copy link
Contributor Author

realead commented Jun 25, 2019

@scoder, Sorry to bother you, but I'm really struggling to understand the test results. It looks as if cythonized cpdef_enums.pyx would not compile:

=== C/C++ compiler output: =========
cpdef_enums.cpp: In function ‘PyObject* PyInit_cpdef_enums()’:
cpdef_enums.cpp:4378:40: error: ‘__pyx_builtin_globals’ was not declared in this scope
   __pyx_t_1 = __Pyx_PyObject_CallNoArg(__pyx_builtin_globals); if (unlikely(!__pyx_t_1)) __PYX_ERR(1, 46, __pyx_L1_error)
                                        ^~~~~~~~~~~~~~~~~~~~~
cpdef_enums.cpp:4378:40: note: suggested alternative: ‘__Pyx_InitGlobals’
   __pyx_t_1 = __Pyx_PyObject_CallNoArg(__pyx_builtin_globals); if (unlikely(!__pyx_t_1)) __PYX_ERR(1, 46, __pyx_L1_error)
                                        ^~~~~~~~~~~~~~~~~~~~~
                                        __Pyx_InitGlobals
cpdef_enums.cpp: In function ‘PyObject* PyInit_cpdef_enums()’:
cpdef_enums.cpp:4378:40: error: ‘__pyx_builtin_globals’ was not declared in this scope
   __pyx_t_1 = __Pyx_PyObject_CallNoArg(__pyx_builtin_globals); if (unlikely(!__pyx_t_1)) __PYX_ERR(1, 46, __pyx_L1_error)
                                        ^~~~~~~~~~~~~~~~~~~~~
cpdef_enums.cpp:4378:40: note: suggested alternative: ‘__Pyx_InitGlobals’
   __pyx_t_1 = __Pyx_PyObject_CallNoArg(__pyx_builtin_globals); if (unlikely(!__pyx_t_1)) __PYX_ERR(1, 46, __pyx_L1_error)
                                        ^~~~~~~~~~~~~~~~~~~~~
                                        __Pyx_InitGlobals
====================================

It builds however locally on Python3.5,3.6,3.7. In the resulting cpp there is no __pyx_builtin_globals I also cannot find anything in the codebase what could produce this name.

Do you have any idea what this happens and what is the issue here at hand?

@TeamSpen210
Copy link
Contributor

That should contain a copy of the globals() Python function. It seems it's not getting redirected to call Cython's implementation of that - the builtin won't work anyway so it's not imported.

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for the globals problem either. Could you rebase your branch on the latest master and push it again, to see if it's just some kind of temporary hickup?

def __call__(self, parser, namespace, values, option_string=None):
directives = getattr(namespace, 'compiler_directives', {})
directives.update(Options.extra_warnings)
setattr(namespace, 'compiler_directives', directives)
Copy link
Contributor

@scoder scoder Jul 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for setattr() here. Just use plain attribute access. (Same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changed.

@realead
Copy link
Contributor Author

realead commented Jul 27, 2019

@scoder still the same problem after rebasing:( still no clue what is going on...

@realead
Copy link
Contributor Author

realead commented Jul 30, 2019

@scoder I'm not sure, why only my PRs are affected by this error - I have tried to find out which commit is responsible (see #3061) and it results in this - which should not make any difference, as it only adds some test cases.

@scoder
Copy link
Contributor

scoder commented Aug 3, 2019

Ah, yes, that explains it. Your command line tests change the settings in Options, so that subsequent tests use a misconfigured compiler and fail.

I see two ways out of this. Either the tests are run in a separate environment (process?) that does not interfere with the test environment, or introduce an intermediate step that collects the settings from argparse but does not apply them yet (which can be tested), and then apply them separately only in the actual command line script. To me, the latter has the disadvantage that things may still go wrong in the settings update step and would then be untested, but the first doesn't seem easy to test either. Although, maybe with a "dump settings" kind of script that uses the command line parser and then prints all settings to stderr? That could be run from the tests.

@realead
Copy link
Contributor Author

realead commented Aug 3, 2019

@scoder thanks, I was relying on setUp/tearDown but they weren't robust enough, so had to improve it first (6d0a055).

The problem was that Options._directive_defaultswas changed here: https://github.com/cython/cython/blob/master/Cython/Compiler/Options.py#L171, which might be a little bit surprising.

@scoder
Copy link
Contributor

scoder commented Aug 4, 2019

Thanks!

@scoder scoder merged commit f019888 into cython:master Aug 4, 2019
@realead
Copy link
Contributor Author

realead commented Aug 4, 2019

@scoder My next step after this PR would be to unify https://github.com/cython/cython/blob/master/Cython/Build/Cythonize.py#L122 and https://github.com/cython/cython/blob/master/Cython/Compiler/CmdLine.py#L72:

There are some options that can be used in both, some only for cythonize and some only for cython. Some currently only-cython-options would probably make sense also for cythonize - after the unification adding them to cythonize would be almost for free.

Also probably it would be good to add some of the options to IPython-magic: right now --3str is missing (https://github.com/cython/cython/blob/master/Cython/Build/IpythonMagic.py#L194) and ther is no way to get the new "default" 3st in IPython (see https://github.com/cython/cython/blob/master/Cython/Build/IpythonMagic.py#L427). But from time to time I also miss others.

However, it still costs some time of yours for answering questions/reviewing so I'm asking for you permission to do so.

@scoder
Copy link
Contributor

scoder commented Aug 7, 2019

I think it's a good idea to merge both.
And IPython should default to 3str, just like everything else now. Then we wouldn't need --3str, really.

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

Successfully merging this pull request may close these issues.

None yet

3 participants