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

ENH: Support "compiling" source files exclusively to .pyc #1192

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MujassimJamal
Copy link

@MujassimJamal MujassimJamal commented Feb 17, 2024

To support “compiling” source file exclusively to .pyc, the CMake function ctkFunctionAddCompilePythonScriptTargets has been improved to accept a new parameter option SKIP_SCRIPT_COPY. This new option controls the definition of the target CopySlicerPythonScriptFiles.


Introduce the option CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC to support removing `*.py` scripts once the corresponding `.pyc` file has been generated in the destination directory.

Update _ctk_add_compile_python_directories_target to accept keep_only_pyc argument.

Update CMake function ctkFunctionAddCompilePythonScriptTargets to accept the KEEP_ONLY_PYC option and call _ctk_add_compile_python_directories_target accordingly.

Update CMake macro ctkMacroCompilePythonScript to call ctkFunctionAddCompilePythonScriptTargets passing KEEP_ONLY_PYC based on the value of CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC.

Address warning when using CMake >= 3.13, setting CMP0077 to NEW.
See https://cmake.org/cmake/help/latest/policy/CMP0077.html

Related pull requests:

Related discussions:

@jcfr jcfr changed the title ENH:Support “compiling” source file exclusively to .pyc ENH: Support "compiling" source file exclusively to .pyc Feb 23, 2024
@jcfr jcfr changed the title ENH: Support "compiling" source file exclusively to .pyc ENH: Support "compiling" source files exclusively to .pyc Feb 24, 2024
@jcfr jcfr force-pushed the mujassim branch 4 times, most recently from d55c93e to 6b0f3fa Compare March 20, 2024 16:13
Introduce the option CTK_COMPILE_PYTHON_SCRIPT_SKIP_SCRIPT_COPY
to support disabling the copy of `*.py` script into the build
directory.

Update CMake function `ctkFunctionAddCompilePythonScriptTargets` to
accept the `SKIP_SCRIPT_COPY` option.

Update CMake macro `ctkMacroCompilePythonScript` to call `ctkFunctionAddCompilePythonScriptTargets`
passing SKIP_SCRIPT_COPY based on the value of CTK_COMPILE_PYTHON_SCRIPT_SKIP_SCRIPT_COPY

Address warning when using CMake >= 3.13, setting CMP0077 to NEW.
See https://cmake.org/cmake/help/latest/policy/CMP0077.html

Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
@jcfr
Copy link
Member

jcfr commented Mar 20, 2024

More work is still need as _ctk_add_compile_python_directories_target currently expect to compile script first copied into the destination directory.

By configuring CTK with CTK_COMPILE_PYTHON_SCRIPT_SKIP_SCRIPT_COPY set to TRUE (starting from an empty build directory), there will be no .py files in the destination directory and an error like the following will be reported:

[ 72%] Performing build step for 'CTK'
make[5]: *** No rule to make target 'Libs/Scripting/Python/Core/Python/CopyCTKScriptingPythonCorePythonScriptFiles', needed by 'Libs/Scripting/Python/Core/Python/python_compile_CTKScriptingPythonCore_complete'.  Stop.
make[4]: *** [CMakeFiles/Makefile2:1308: Libs/Scripting/Python/Core/Python/CMakeFiles/CompileCTKScriptingPythonCorePythonFiles.dir/all] Error 2
make[4]: *** Waiting for unfinished jobs....
[ 51%] Built target CTKCore
make[3]: *** [Makefile:146: all] Error 2
make[2]: *** [CMakeFiles/CTK.dir/build.make:87: CTK-prefix/src/CTK-stamp/CTK-build] Error 2
make[1]: *** [CMakeFiles/Makefile2:896: CMakeFiles/CTK.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

We will need to address this before moving forward. I will follow-up with some suggestions.

@jcfr jcfr marked this pull request as draft March 20, 2024 16:50
Introduce the option CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC
to support remove `*.py` scripts once the corresponding `.pyc`
file has been generated in the destination directory.

Update `_ctk_add_compile_python_directories_target` to accept `keep_only_pyc` argument.

Update CMake function `ctkFunctionAddCompilePythonScriptTargets` to
accept the `KEEP_ONLY_PYC` option and call `_ctk_add_compile_python_directories_target`
accordingly.

Update CMake macro `ctkMacroCompilePythonScript` to call `ctkFunctionAddCompilePythonScriptTargets`
passing KEEP_ONLY_PYC based on the value of `CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC`.

Address warning when using CMake >= 3.13, setting CMP0077 to NEW.
See https://cmake.org/cmake/help/latest/policy/CMP0077.html

Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
@jcfr jcfr marked this pull request as ready for review March 20, 2024 18:43
@jcfr
Copy link
Member

jcfr commented Mar 20, 2024

I revisited the approach, suggested commit message for squash & Merge:

ENH: Support "compiling" source file exclusively to .pyc

Introduce the option CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC
to support removing `*.py` script once the corresponding `.pyc`
file has been generated in the destination directory.

Update `_ctk_add_compile_python_directories_target` to accept `keep_only_pyc` argument.

Update CMake function `ctkFunctionAddCompilePythonScriptTargets` to
accept the `KEEP_ONLY_PYC` option and call `_ctk_add_compile_python_directories_target`
accordingly.

Update CMake macro `ctkMacroCompilePythonScript` to call `ctkFunctionAddCompilePythonScriptTargets`
passing KEEP_ONLY_PYC based on the value of `CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC`.

Address warning when using CMake >= 3.13, setting CMP0077 to NEW.
See https://cmake.org/cmake/help/latest/policy/CMP0077.html

Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>

@jcfr
Copy link
Member

jcfr commented Mar 20, 2024

@MujassimJamal Could you test on your side ? 🙏

@jamesobutler @lassoan I revisited the original approach to instead accept the option CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC and delete the file once it was successfully compiled:

To illustrate here is a screenshot of the generated1 script:

Libs/Scripting/Python/Core/Python/compile_CTKScriptingPythonCore_python_scripts.py

called by the Compile<Name>PythonFiles custom target (e.g CompileCTKScriptingPythonCorePythonFiles or CompileSlicerPythonFiles)

The code highlighted in red is included only if CTK_COMPILE_PYTHON_SCRIPT_KEEP_ONLY_PYC is ON

image

Footnotes

  1. Generated configuring CMake/ctk_compile_python_scripts.cmake.in

@MujassimJamal
Copy link
Author

Thanks, @jcfr, for proposing an alternative approach.

Indeed, the previous approach of skipping the copy target created issues for me during scratch builds, as you mentioned that compiled files must be copied to the destination first. Since then, I've been using an unofficial hard-coded temporary solution, which involves completely removing 'py' files once they are compiled. To achieve this, I directly used the following code snippet in the file ctk_compile_python_scripts.cmake.in:

if 'qt-scripted-modules' in fullname:
    print('Removing', fullname, '...')
    os.remove(fullname)

I'm okay with the alternative approach you proposed, as it allows explicit control over the behavior. I'll test it from my side and provide an update here. 🚀

@jcfr
Copy link
Member

jcfr commented Apr 2, 2024

I'm okay with the alternative approach you proposed

Were you able to test in the context of your project ?

@MujassimJamal
Copy link
Author

MujassimJamal commented Apr 5, 2024

@jcfr Thank you for your patience.

Yes, I've recently tested it and encountered numerous errors in the Python console. The majority of these errors were related to:

  • Failed to instantiate modules
  • qSlicerPythonCppAPI::instantiateClass Failed to instantiate scripted PythonQt class
  • NameError: name {'getSlicerRCFileName', 'loadSlicerRCFile'} is not defined

When I open the application after a build, only a blank main window is visible; all the widgets are gone (e.g slice views, 3D view, module panel, etc.).

Perhaps this occurred because it removes ".py" files not only from the lib/{AppName}/qt-scripted-modules directory but also from other directories (e.g. bin etc.) within the Slicer-build.

@MujassimJamal
Copy link
Author

@jcfr, I want to know if you are planning to proceed with this PR?

@jcfr
Copy link
Member

jcfr commented May 16, 2024

Based on your tests1, Slicer would need to also be updated to make this feature useful.

Would you be able to open a pull request against Slicer do address those?

Footnotes

  1. https://github.com/commontk/CTK/pull/1192#issuecomment-2038796075

@MujassimJamal
Copy link
Author

Sure, I will open a pull request with necessary changes to the Slicer.

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

Successfully merging this pull request may close these issues.

2 participants