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

MapScript PHPNG adding false build targets to Python MapScript #5708

Closed
geographika opened this Issue Nov 24, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@geographika
Copy link
Member

geographika commented Nov 24, 2018

The following code in the mapscript.i file:

#ifdef SWIGPHPNG
%module mapscriptng
#else
%module mapscript
#endif

Causes the following to be added to build/mapscript/python/CMakeFiles/_pythonmapscript.dir/build.make

mapscript/python/mapscriptng.py: mapscript/python/mapscriptPYTHON_wrap.c
	@$(CMAKE_COMMAND) -E touch_nocreate mapscript/python/mapscriptng.py 

As this file is never generated it has the following knock-on effect of generating a dependency that is never created in /build/mapserver/build/mapscript/python/CMakeFiles/_pythonmapscript.dir/DependInfo.Cmake

# Pairs of files generated by the same build rule.
set(CMAKE_MULTIPLE_OUTPUT_PAIRS
  "/build/mapserver/build/mapscript/python/mapscriptng.py" "/build/mapserver/build/mapscript/python/mapscriptPYTHON_wrap.c"
  )

This causes the Python MapScript bindings to be rebuilt every time make or make install is called as shown in the logs (for future reference you can see what triggered the rebuild using make install VERBOSE=1:

make[2]: Entering directory '/build/mapserver/build'
cd /build/mapserver/build && /usr/bin/cmake -E cmake_depends "Unix Makefiles" /build/mapserver /build/mapserver/mapscript/python /build/mapserver/build /build/mapserver/build/mapscript/python /build/mapserver/build/mapscript/python/CMakeFiles/_pythonmapscript.dir/DependInfo.cmake --color=
Deleting primary custom command output "/build/mapserver/build/mapscript/python/mapscriptPYTHON_wrap.c" because another output "/build/mapserver/build/mapscript/python/mapscriptng.py" does not exist.

Using the following command - grep -iRl "mapscriptng" ./ the only place mapscriptng is mentioned is in mapscript.i

I'm not sure on how everything fits together, or why this target is generated by CMake. Changing it to the following fixes the issue in Python, but may break the PHPNG build:

%module mapscript
#ifdef SWIGPHP
%module mapscriptng
#endif

@AlexanderGabriel - maybe you have an idea on what causes this?

@AlexanderGabriel

This comment has been minimized.

Copy link
Contributor

AlexanderGabriel commented Nov 25, 2018

Hi @geographika ,

sure, you are in the right branch?
https://github.com/mapserver/mapserver/blob/master/mapscript/mapscript.i#L33
There is "#ifdef SWIGPHP".
However... The code there should not be parsed if SWIGPHP is not defined and it should not cause a rebuild everey time.
I renamed swig based mapscript to mapscriptng to be able to load both modules at the same time and to make visible that this extension is not the old one.
"mapscriptng" is used in branch master:
grafik

If this line of code causes any problems (which should not be. this is a precompiler directive and this code shout be ignored, maybe another bug?), maybe it works to do ifdef and ifndef instead of if else:
#ifdef SWIGPHP
%module mapscriptng
#endif
#ifndef SWIGPHP
%module mapscript
#endif
But i did not test this.
Did you try with a newer SWIG?

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Nov 25, 2018

@AlexanderGabriel - thanks for the reply. The code looks fine to me with the #ifdef in mapscript.i but it generates an incorrect build target. It could well be some CMake/SWIG issue/bug. Apparently SWIG implements some of the C pre-processor functionality, but is not the preprocessor. Also %module% has to be at the top of the file (but I've seen examples on GitHub also using #ifdef.

The pull request #5709 is based on master so should have all the latest PHP changes. My change at 4bfd0c5 gets rid of the incorrect build rule, but likely breaks the PHPNG MapScript build.

Travis builds correctly - https://travis-ci.org/mapserver/mapserver/builds/459243895 and uses cmake version 3.9.2, however it has * PHPNG: disabled - should these be run normally?

I'm using cmake 3.10.2 on Ubuntu where the error occurs. I'll give the ifdef and ifndef suggestion above a try, and also see if the same issue happens on Windows.

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Nov 25, 2018

Some updates on this:

  • there is no mention of mapscriptng in any files produced on Windows (when building without any PHP MapScripts)
  • Changing to the following does not resolve the issue - mapscriptng.py is still in the CMake dependencies
#ifdef SWIGPHP
%module mapscriptng
#endif
#ifndef SWIGPHP
%module mapscript
#endif
  • removing the entire contents of mapscript/phpng/CMakeLists.txt has no effect (issue remains), so I guess that rules out any command in there (same for /build/mapserver/mapscript/php/CMakeLists.txt).

If you get a chance maybe you could confirm the issue exists in your setup? Build with DWITH_PYTHON=ON and DWITH_PHPNG=OFF, and see if grep -iRl "mapscriptng" ./ returns any files?

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Nov 26, 2018

Ok I'm pretty sure this is a CMake/SWIG issue. It looks like a regex is used to find the module name:

file ( STRINGS "${infile}" module_basename REGEX "[ ]*%module[ ]*[a-zA-Z0-9_]+.*" )

https://github.com/Kitware/CMake/blob/master/Modules/UseSWIG.cmake#L289

It should be possible to change the name of the module when calling SWIG - see possibly related #5699 using something like the following:

set_property(SOURCE my_swig_module.i PROPERTY SWIG_MODULE_NAME my_swig_module)
However there may be a bug that means it is not possible:

https://gitlab.kitware.com/cmake/cmake/issues/18374

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Nov 26, 2018

Looking at the UseSWIG code there is an easy way to stop this happening, by switching the order %module% appears in mapscript.i:

#ifndef SWIGPHPNG
%module mapscript
#else
%module mapscriptng
#endif

This may well break the PHPNG module name though.

@AlexanderGabriel

This comment has been minimized.

Copy link
Contributor

AlexanderGabriel commented Nov 27, 2018

Tested on my machine, could compile extension and load it, could create a mapObj and access properties.
Looks good to me. Nothing broken.

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Nov 27, 2018

@AlexanderGabriel - thanks for testing. I'll apply this update then if it is ok with you.
Quick question - if you run make twice does it rebuild the PHPNG module, even though it is built already? That was the main issue with the Python MapScript bindings - they were build every time, including on make install.

@AlexanderGabriel

This comment has been minimized.

Copy link
Contributor

AlexanderGabriel commented Nov 27, 2018

@geographika i dont use

make

;)
i use

cmake --build .

on both, windows and linux. did not test on linux but on windows there is no rebuilding

geographika added a commit to geographika/mapserver that referenced this issue Nov 27, 2018

@geographika

This comment has been minimized.

Copy link
Member Author

geographika commented Dec 15, 2018

Closed with #5713

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.