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

configure: Support for libpng-config #2679

Merged
merged 13 commits into from
Dec 6, 2022

Conversation

HuidaeCho
Copy link
Member

@HuidaeCho HuidaeCho commented Dec 2, 2022

This PR is needed to use the OSGeo4W version of libpng, not the MSYS2 version.

Usage:

OSGEO4W_ROOT_MSYS=/c/OSGeo4W \
./configure \
--libpng-config=mswindows/osgeo4w/libpng-config

The included libpng-config is version-agnostic as long as ${OSGEO4W_ROOT_MSYS}/include/libpng${PNG_DLL_VERSION}/png.h exists.

@HuidaeCho HuidaeCho added the windows Microsoft Windows specific label Dec 2, 2022
@HuidaeCho HuidaeCho changed the title configure: PNGVER environment variable to pickup libpng16.dll from OSGeo4W v2 configure: PNGVER environment variable to pick up libpng16.dll from OSGeo4W v2 Dec 2, 2022
@HuidaeCho
Copy link
Member Author

HuidaeCho commented Dec 2, 2022

Maybe, adding support for libpng-config and creating mswindows/osgeo4w/libpng-config would be better...

@HuidaeCho HuidaeCho changed the title configure: PNGVER environment variable to pick up libpng16.dll from OSGeo4W v2 configure: Checking for libpng{VERSION}.dll from OSGeo4W v2 Dec 2, 2022
@HuidaeCho
Copy link
Member Author

BTW, is libpng-config included in most Linux distributions and macOS? Safe to remove --with-png-(includes|libs)?

@HuidaeCho
Copy link
Member Author

HuidaeCho commented Dec 4, 2022

HAVE_PNG_H is not defined for Ubuntu gcc?
https://github.com/OSGeo/grass/actions/runs/3610010032/jobs/6083548001#step:10:160

ERROR: write_image: unknown file type: .png

from

#ifdef HAVE_PNG_H
else if (G_strcasecmp(p, ".png") == 0)
write_png();
#endif
else
G_fatal_error("write_image: unknown file type: %s", p);

@HuidaeCho HuidaeCho changed the title configure: Checking for libpng{VERSION}.dll from OSGeo4W v2 configure: Support for libpng-config Dec 4, 2022
@HuidaeCho
Copy link
Member Author

libpng-config from Ubuntu 20.04 seems to be broken. It doesn't print an -L flag for --ldflags.

...
libs="-lpng16"
...
L_opts=
R_opts=
...
ldopts=""
...
    --ldflags)
        echo ${ldopts} ${L_opts} ${R_opts} {$libs}

@HuidaeCho HuidaeCho added the enhancement New feature or request label Dec 4, 2022
nilason
nilason previously requested changes Dec 6, 2022
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to change HAVE_PNG_H to HAVE_PNG in the files:

#ifdef HAVE_PNG_H

#ifdef HAVE_PNG_H

And then it works for me.

There is one difference to PNGLIB compared to before:

was -L/opt/local/lib -lpng -lz
with this: -L/opt/local/lib -lpng16

there is a missing -lz flag. I'm not sure of the importance or consequence of this.

configure.ac Outdated Show resolved Hide resolved
@nilason
Copy link
Contributor

nilason commented Dec 6, 2022

HAVE_PNG_H is not defined for Ubuntu gcc?

HAVE_PNG_H was defined before with: LOC_CHECK_INCLUDES(png.h,PNG,$PNGINC), but not with this PR. HAVE_PNG serves the same purpose.

configure Show resolved Hide resolved
@HuidaeCho
Copy link
Member Author

there is a missing -lz flag. I'm not sure of the importance or consequence of this.

Not sure either, but if -lz is missing from the system libpng-config, maybe harmless.

@HuidaeCho HuidaeCho dismissed nilason’s stale review December 6, 2022 14:51

Changing HAVE_PNG to HAVE_PNG_H is more consistent with other *_H and safer (previously it was HAVE_PNG_H)

@nilason
Copy link
Contributor

nilason commented Dec 6, 2022

there is a missing -lz flag. I'm not sure of the importance or consequence of this.

Not sure either, but if -lz is missing from the system libpng-config, maybe harmless.

Well, it is there. Using libpng-config --static --ldflags I get -L/opt/local/lib -lpng16 -lz.

it comes from:
all_libs="-lpng16 -lz "
originating from libpng source file libpng-config.in:
all_libs="-lpng@PNGLIB_MAJOR@@PNGLIB_MINOR@ @LIBS@"

$ZLIB="-lz" and $MATHLIB="" (on my system) was previously set as mandatory (and added) by autoconf with:
LOC_CHECK_LIBS(png,png_read_image,PNG,$PNGLIB,PNGLIB,,$ZLIB $MATHLIB,)
see:

grass/aclocal.m4

Lines 145 to 159 in e8f26dd

dnl $1 = library
dnl $2 = function
dnl $3 = descriptive name
dnl $4 = LDFLAGS initialiser
dnl $5 = result variable
dnl $6 = mandatory dependencies (not added to $5)
dnl $7 = mandatory dependencies (added to $5)
dnl $8 = ACTION-IF-NOT-FOUND
dnl $9+ = optional dependencies
define(LOC_CHECK_LIBS_0,[
AC_CHECK_LIB($1, $2, $5="$$5 -l$1 $7",[
[$8]
],$6 $7)
])

Maybe libpng-config --static --ldflags could be used instead of libpng-config --ldflags?

Re: HAVE_PNG vs HAVE_PNG_H. There is already a bunch of similar to the former one with e.g. HAVE_OGR, HAVE_GDAL, HAVE_PDAL. But no big issue, works either way.

@HuidaeCho
Copy link
Member Author

libpng-config --static --ldflags

That's interesting.. Only --static --ldflags includes -lm -lz. This is my output from Slackware:

-L/usr/lib64 -lpng16 -lm -lz -lm

ldd /usr/lib64/libpng16.so actually produces

        linux-vdso.so.1 (0x00007fff14bd0000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f4860828000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f48606e0000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f4860501000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f48608ae000)

including libz.so.1 and libm.so.6, so I guess libpng-config --static --ldflags would be safer although we're not really doing "static linking".

@nilason
Copy link
Contributor

nilason commented Dec 6, 2022

so I guess libpng-config --static --ldflags would be safer although we're not really doing "static linking".

Indeed, that seems to be the safest way not to introduce regression.

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@HuidaeCho HuidaeCho merged commit 8efd228 into OSGeo:main Dec 6, 2022
@HuidaeCho HuidaeCho deleted the configure-png-version branch December 6, 2022 21:44
nilason added a commit to nilason/grass that referenced this pull request Dec 9, 2022
Last minute changes to configure.ac with OSGeo#2679 never was
generated to configure.
nilason added a commit that referenced this pull request Dec 9, 2022
Last minute changes to configure.ac with commit
8efd228
(#2679) was never generated to configure.
@neteler neteler added this to the 8.3.0 milestone Feb 13, 2023
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* configure: PNGVER environment variable to pickup libpng16.dll from OSGeo4W v2

* Add libpng-config (part of libpng) support and missing script for OSGeo4W

* Add --dll-version to libpng-config; Update gdal-config to detect libpng version

* Add --libpng-config to build_centos.sh

* Fix for clang (no implicit declaration of function)

* Use libpng-config --ldflags instead of --libs to include -L flags

* Let's try LDFLAGS for Ubuntu 20.04

* Define HAVE_PNG_H

* Consistent comments

* Delete trailing spaces; Replace tabs+spaces+tabs with tabs+spaces

* Copyright year to 2022

* Revert build_ubuntu-20.04.sh

* Add --static flag to libpng-config to include -lm -lz

Co-authored-by: Huidae Cho <grass4u@gmail.com>
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
Last minute changes to configure.ac with commit
8efd228
(OSGeo#2679) was never generated to configure.
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* configure: PNGVER environment variable to pickup libpng16.dll from OSGeo4W v2

* Add libpng-config (part of libpng) support and missing script for OSGeo4W

* Add --dll-version to libpng-config; Update gdal-config to detect libpng version

* Add --libpng-config to build_centos.sh

* Fix for clang (no implicit declaration of function)

* Use libpng-config --ldflags instead of --libs to include -L flags

* Let's try LDFLAGS for Ubuntu 20.04

* Define HAVE_PNG_H

* Consistent comments

* Delete trailing spaces; Replace tabs+spaces+tabs with tabs+spaces

* Copyright year to 2022

* Revert build_ubuntu-20.04.sh

* Add --static flag to libpng-config to include -lm -lz

Co-authored-by: Huidae Cho <grass4u@gmail.com>
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Last minute changes to configure.ac with commit
8efd228
(OSGeo#2679) was never generated to configure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request windows Microsoft Windows specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants