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

Add conv-template and from-template scripts #278

Closed
wants to merge 7 commits into
base: master
from

Conversation

2 participants
@ghost

ghost commented Feb 15, 2018

These scripts are required to generate *.src sources present in NumPy and SciPy.

cc @jcfr

xoviat added some commits Feb 15, 2018

xoviat
@jcfr

This comment has been minimized.

Contributor

jcfr commented Feb 16, 2018

Thanks for the patch 👍

You will find few comments posted below.

# ``NumPy_CONV_TEMPLATE``
# The command-line arguments required to execute the conv-template script
# ``NumPy_FROM_TEMPLATE``
# The command-line arguments required to execute the from-template script

This comment has been minimized.

@jcfr

jcfr Feb 16, 2018

Contributor

I suggest to name the variables NumPy_CONV_TEMPLATE_EXECUTABLE and NumPy_FROM_TEMPLATE_EXECUTABLE.

Since the variable is set to the path to the executable, should the description be updated to something like this:

Path to the conv-template script

This comment has been minimized.

@ghost

ghost Feb 16, 2018

Well it's not necessarily the executable. It's supposed to be one or two arguments that you add to the beginning of the command line.

This comment has been minimized.

@jcfr

jcfr Feb 16, 2018

Contributor

Since conv-template and from-template are python scripts, using find_program will not work.

Instead I suggest the following approach:

if(NOT DEFINED NumPy_CONV_TEMPLATE_SCRIPT)
  execute_process(COMMAND "${PYTHON_EXECUTABLE}"
       -c "from numpy.distutils import conv_template; print(conv_template.__file__)"
    OUTPUT_VARIABLE _numpy_conv_template_file
    OUTPUT_STRIP_TRAILING_WHITESPACE
    ERROR_QUIET
  )
  set(NumPy_CONV_TEMPLATE_SCRIPT "${_numpy_conv_template_file}" CACHE FILEPATH "path to conv-template script")
endif()

...

This comment has been minimized.

@ghost

ghost Feb 16, 2018

These scripts will be present in future releases of NumPy though (see here). The execute_process is for backward compatibility only.

xoviat
@codecov-io

This comment has been minimized.

codecov-io commented Feb 16, 2018

Codecov Report

Merging #278 into master will decrease coverage by 0.95%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
- Coverage   89.86%   88.91%   -0.96%     
==========================================
  Files          25       25              
  Lines         947      947              
  Branches      160      160              
==========================================
- Hits          851      842       -9     
- Misses         72       76       +4     
- Partials       24       29       +5
Impacted Files Coverage Δ
skbuild/platform_specifics/windows.py 66.66% <0%> (-6.07%) ⬇️
skbuild/cmaker.py 69.44% <0%> (-2.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cd5967...81de5ec. Read the comment docs.

xoviat
@jcfr

This comment has been minimized.

Contributor

jcfr commented Feb 16, 2018

This approach would give the most flexibility:

if(NOT DEFINED NumPy_CONV_TEMPLATE_SCRIPT)
  execute_process(COMMAND "${PYTHON_EXECUTABLE}"
       -c "from numpy.distutils import conv_template; print(conv_template.__file__)"
    OUTPUT_VARIABLE _numpy_conv_template_file
    OUTPUT_STRIP_TRAILING_WHITESPACE
    ERROR_QUIET
  )
  set(NumPy_CONV_TEMPLATE_SCRIPT "${_numpy_conv_template_file}" CACHE FILEPATH "path to conv-template script")
endif()

On the other hand, if having both the python interpreter and the script packed into one variable is important, the variable could be named NumPy_CONV_TEMPLATE_COMMAND

@ghost

This comment has been minimized.

ghost commented Feb 16, 2018

These scripts will be present in future releases of NumPy though (see here). The execute_process is for backward compatibility only.

xoviat added some commits Feb 16, 2018

xoviat
xoviat
@jcfr

This comment has been minimized.

Contributor

jcfr commented Feb 16, 2018

Gotcha.

Since this is for backward compatibility .... naming the variable with _EXECUTABLE prefix make sense. (Sorry for the back and forth ...)

Let's also make sure to set the variable into the cache.

Here is what I suggest:

  find_program(NumPy_CONV_TEMPLATE_EXECUTABLE NAMES conv-template)
  find_program(NumPy_FROM_TEMPLATE_EXECUTABLE NAMES from-template)

  if(PYTHON_EXECUTABLE)
   
    [...]

    # XXX To support NumPy < v0.15.0 where conv-template is not declared as an entry point, we
    # emulate the behavior of a standalone executable using the path the the python interpreter and the 
    # associated conv-template script.
    if(NOT NumPy_CONV_TEMPLATE_EXECUTABLE)
      execute_process(COMMAND "${PYTHON_EXECUTABLE}"
        -c "from numpy.distutils import conv_template; print(conv_template.__file__)"
        OUTPUT_VARIABLE _numpy_conv_template_file
        OUTPUT_STRIP_TRAILING_WHITESPACE
        ERROR_QUIET
        )
      set(NumPy_CONV_TEMPLATE_EXECUTABLE "${PYTHON_EXECUTABLE}" "${_numpy_conv_template_file}" CACHE STRING "Command executing conv-template program")
    endif()

    # XXX To support NumPy < v0.15.0 where from-template is not declared as an entry point, we
    # emulate the behavior of a standalone executable using the path the the python interpreter and the 
    # associated from-template script.
    if(NOT NumPy_FROM_TEMPLATE_EXECUTABLE)
      execute_process(COMMAND "${PYTHON_EXECUTABLE}"
        -c "from numpy.distutils import from_template; print(from_template.__file__)"
        OUTPUT_VARIABLE _numpy_from_template_file
        OUTPUT_STRIP_TRAILING_WHITESPACE
        ERROR_QUIET
        )
      set(NumPy_FROM_TEMPLATE_EXECUTABLE "${PYTHON_EXECUTABLE}" "${_numpy_from_template_file}" CACHE STRING "Command executing from-template program")
    endif()
  endif()
endif()
xoviat
@ghost

This comment has been minimized.

ghost commented Feb 16, 2018

Does that look okay?

@jcfr

This comment has been minimized.

Contributor

jcfr commented Feb 16, 2018

Closing. Integrated in 7abf44c

@jcfr jcfr closed this Feb 16, 2018

@ghost ghost deleted the find-conv-template branch Feb 16, 2018

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