Compile time env available from cython command line options #2315
Conversation
| x_args = pop_value() | ||
| try: | ||
| options.compile_time_env = Options.parse_compile_time_env( | ||
| x_args, current_settings=options.compile_time_env) |
There was a problem hiding this comment.
Any reason the current_settings are only passed here and not in Cythonize.py?
Cython/Compiler/Options.py
Outdated
| >>> parse_compile_time_env(' ') | ||
| {} | ||
| >>> (parse_compile_time_env('boundscheck=True') == | ||
| ... {'boundscheck': True}) |
There was a problem hiding this comment.
This is not a good example, because it suggests directive names to be evaluated. The compile time env has nothing to do with directives.
There was a problem hiding this comment.
I agree, I'll fix that with dummy variable names
There was a problem hiding this comment.
I updated the doctest to be more realistic and less misleading for other developers.
Cython/Compiler/Options.py
Outdated
| continue | ||
| if '=' not in item: | ||
| raise ValueError('Expected "=" in option "%s"' % item) | ||
| name, value = [s.strip() for s in item.strip().split('=', 1)] |
There was a problem hiding this comment.
No need for stripping the original string here, since we need to strip each part anyway.
| @@ -423,7 +423,7 @@ def parse_directive_list(s, relaxed_bool=False, ignore_unknown=False, | |||
| item = item.strip() | |||
| if not item: | |||
| continue | |||
| if not '=' in item: | |||
| if '=' not in item: | |||
There was a problem hiding this comment.
It's a bit sad that these changes slipped in. Probably a commit hook on your side or so.
While I think that cleaning them up is a good thing, it should better not happen as part of an unrelated commit.
Cython/Build/Cythonize.py
Outdated
| @@ -139,6 +139,9 @@ def parse_args(args): | |||
| parser.add_option('-X', '--directive', metavar='NAME=VALUE,...', dest='directives', | |||
| type=str, action='callback', callback=parse_directives, default={}, | |||
| help='set a compiler directive') | |||
| parser.add_option('-E', '--compile-time-env', metavar='NAME=VALUE,...', dest='compile_time_env', | |||
| type=str, action='callback', callback=Options.parse_compile_time_env, default={}, | |||
| help='set a compile time environment variables') | |||
Cython/Build/Cythonize.py
Outdated
| @@ -139,6 +139,9 @@ def parse_args(args): | |||
| parser.add_option('-X', '--directive', metavar='NAME=VALUE,...', dest='directives', | |||
| type=str, action='callback', callback=parse_directives, default={}, | |||
| help='set a compiler directive') | |||
| parser.add_option('-E', '--compile-time-env', metavar='NAME=VALUE,...', dest='compile_time_env', | |||
| type=str, action='callback', callback=Options.parse_compile_time_env, default={}, | |||
There was a problem hiding this comment.
I can see in the surrounding code where this is coming from, but it seems wrong to say type=str when the actual value is a dict. And I'm also not sure that this allows passing -E multiple times.
There was a problem hiding this comment.
The type is the input type, before the action of the callback.
Setting it to "dict" would require the definition of what is a dict in OptParser, how to parse it, ... and duplicate most of the code already present.
When not defining it, the callback gets None instead of "" and most of them fail in parsing the string.
I am not very familiar with cythonize in the command line but apparently it works now as well.
|
I'm a bit torn whether |
|
On Fri, 08 Jun 2018 23:14:01 -0700 scoder ***@***.***> wrote:
I'm a bit torn whether `-E` is better than `-D`, but I'm not entirely sure that we'll never want to use `-D` to pass macro definitions down to the C compiler in `cythonize`, so ...
unfortunately -D is already used for no doc-strings ... the alternative
I see is -Y as -X is for compiler directives...
I'll fix the docstrings for tests to make them more explicit.
Concerning the 'not "=" in', pep8 recommands to replace it with its
direct equivalent. Having a specific PR for this tend to reduce the
agility of the project. but as you want, you are the reviewer :)
Cheers
Jerome
|
|
The spacing/PEP8 changes don't need to be a separate PR, really, but they should not be part of an unrelated commit. Having them as a separate commit would be ok. (I admit that I'm guilty of doing such cleanups myself from time to time, and not always taking the time to separate them out either. We're all human...) |
|
What I'm saying is: my other comments are more important than the one about the style changes. :) |
I used the "-E" short option but if you believe other letters are better suited, I can change.
close #2314