Opened 8 years ago

Closed 8 years ago

#6431 closed defect (fixed)

mkgdaldist.sh fails with Perl bindings

Reported by: Even Rouault Owned by: Ari Jolma
Priority: normal Milestone:
Component: PerlBindings Version: unspecified
Severity: normal Keywords:
Cc:

Description

Change History (13)

comment:1 by Ari Jolma, 8 years ago

Resolution: fixed
Status: newclosed

cpl_config.h is needed because the bindings need to have correct GIntBig and GUIntBig typedefs.

The problem is that that part needs to be put into %{ .. %} so that swig will just transfer it into the bindings and not look into it more.

Regarding undefined $ARGV{"--gdal-source-tree"} it is a silly hack to test it against '0'. The idea was, I think, to test downloading the GDAL source (extreme bootstrapping the Perl bindings that build robots do). It probably got into the trunk by mistake.

Fixed in r33855.

comment:2 by Even Rouault, 8 years ago

Resolution: fixed
Status: closedreopened

r33855 breaks the build of the various targets. For example https://s3.amazonaws.com/archive.travis-ci.org/jobs/120240267/log.txt :

t/00.t .............. 1/? TypeError in method 'SetCacheMax', argument 1 of type 'GIntBig'
# Looks like your test exited with 255 just after 33.

                            
t/00.t .............. Dubious, test returned 255 (wstat 65280, 0xff00)
All 33 subtests passed 

t/alg.t ............. 1/7 
#   Failed test 'polygonize made 2 features'
#   at t/alg.t line 19.
# Looks like you failed 1 test of 7.

                           
t/alg.t ............. Dubious, test returned 1 (wstat 256, 0x100)

t/feature.t ......... 1/? 
#   Failed test 'Set and get integer64 field.'
#   at t/feature.t line 98.
# Looks like you failed 1 test of 25.

                            
t/feature.t ......... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/25 subtests 

Do I understand well that the intent of r33855 is not to require cpl_config.h at bindings .cpp generation time (so mkgdaldist.sh will not need to run ./configure), but only at compilation time ?

comment:3 by Ari Jolma, 8 years ago

auts

Nevermind the r33855, which is obviously a wrong solution to the problem, which is even more subtle than I (too quickly) assumed.

The issue is the inclusion of cpl_config.h, which gives the correct defines, so that swig(?) knows what GIntBig and GUIntBig really are (in the current compiler).

in reply to:  3 comment:4 by Even Rouault, 8 years ago

The issue is the inclusion of cpl_config.h, which gives the correct defines, so that swig(?) knows what GIntBig and GUIntBig really are (in the current compiler).

If the Perl bindings need cpl_config.h to generate the .cpp files, then it means they are dependant of the host target and that we can't ship them in the source archive. Which is not necessarily a huge problem ? Where I'm a bit surprised is that the other bindings don't need SWIG to include cpl_config.h to be able to handle GIntBig. For example in swig/include/python/typemaps_python.i there's a "%typemap(in) GIntBig" that maps to Python C type PY_LONG_LONG

comment:5 by Ari Jolma, 8 years ago

Yes. I see that too. But GIntBig itself depends on the host system. It can be long or long long for example. I've built that logic into the typemaps. Python seems to assume it is always long long?

comment:6 by Ari Jolma, 8 years ago

Maybe it is not needed, but if you have a pointer to long long it can't be used as a pointer to long?

comment:7 by Even Rouault, 8 years ago

I think Python PY_LONG_LONG might expend to "long long" or "int64", but that's Python headers business (and done in C code). So basically ideally, if doable, for the Perl bindings GIntBig should be handled as an opaque/non-standard type. Or perhaps there's a standard way of dealing with 64bit integers in SWIG / Perl SWIG ?

comment:8 by Ari Jolma, 8 years ago

Yes. I think that is correct. Swig has built-in typemaps for 64bit integers for Perl and I'm just letting it use them.

comment:9 by Ari Jolma, 8 years ago

I retracted r33855 in r33856. I would be inclided to leave the wrappers out of the tarball.

comment:10 by Ari Jolma, 8 years ago

I have now replicated the bigint typemaps in the Perl bindings. The tests are ok and the config.h is not needed any more. Let's try this one.

in reply to:  9 comment:11 by Even Rouault, 8 years ago

I would be inclided to leave the wrappers out of the tarball.

I'm fine with that. Would you mind modifying mkgdaldist.sh for that then ? And make sure ./configure --with-perl; make actually works from the generated tarball (not sure why I got an error yesterday)

in reply to:  10 comment:12 by Even Rouault, 8 years ago

Replying to ajolma:

I have now replicated the bigint typemaps in the Perl bindings. The tests are ok and the config.h is not needed any more. Let's try this one.

Oh ok, so let forget my comment about modifying mkgdaldist.sh. Just one question, seeing r33859 and the use of "%lld" and strtoull (might work, but this isn't in the C89 standard according to the man page), I 'm not sure this will be portable on Windows. Why not using CPL_FRMT_GIB instead of "%lld" ? And CPLScanUIntBig() instead of strtoull()

comment:13 by Ari Jolma, 8 years ago

Resolution: fixed
Status: reopenedclosed

Fixed in r33859 and r33863.

Note: See TracTickets for help on using tickets.