Closed kloczek closed 7 months ago
ctruta
commented
7 months ago Hello, and thank you for your report.
It appears that you are running your build after a libpng source upgrade in an already-configured environment. However, due to the recent changes in scripts/pnglibconf.dfa, you need to reconfigure.
You can verify the correct behaviour of a freshly-configured build by running
CI_MAKE="/usr/bin/make" CI_MAKE_FLAGS="-O -j48 V=1 VERBOSE=1" ./ci/ci_verify_configure.sh
kloczek
commented
7 months ago ctruta
commented
7 months ago I just pushed a new commit to print a warning about what went wrong for you, @kloczek. Other users might run into the same issue, so the new warning that I've just added might give a heads-up about what's going on. See commit c9ac5bdba40fa59ec85b16100203ead76e4ebf66.
Apparently, configure script refuses to run in an out-of-tree directory if there is an existing configuration in-tree. You can see it in this error message, last line from your log:
configure: error: source directory already configured; run "make distclean" there firstYou still have the old configuration, from the old libpng in the same directory, and your build process is picking that up. You must delete the old (incompatible) configuration before you recreate a new one:
$ cd /home/tkloczko/rpmbuild/BUILD/libpng-1.6.42/
$ make distcleanI just pushed a new commit to print a warning about what went wrong for you, @kloczek. Other users might run into the same issue, so the new warning that I've just added might give a heads-up about what's going on. See commit c9ac5bd.
Apparently, configure script refuses to run in an out-of-tree directory if there is an existing configuration in-tree. You can see it in this error message, last line from your log:
configure: error: source directory already configured; run "make distclean" there first
But when this needs to be executed? My build current procedure
%prep
%autosetup -p1
%__cp %{S:1} .
%build
autoreconf -fiv
%configure \
--disable-static \
--enable-hardware-optimizations \
%{nil}
%make_build DFA_XTRA=pngusr.dfa
%install
%make_install
%check
CI_MAKE="/usr/bin/make" CI_MAKE_FLAGS="-O -j48 V=1 VERBOSE=1" ./ci/ci_verify_configure.sh
%make_build checkYou still have the old configuration, from the old libpng in the same directory, and your build process is picking that up. You must delete the old (incompatible) configuration before you recreate a new one:
How it is possible? Does it mean that dist tarball comes with "old configuration"?🤔
Execution of the "make distclean" after confiture causes that build fails
ctruta
commented
7 months ago Execution of the "make distclean" after confiture causes that build fails
If you have an existing configured build, then make distclean will clean that up. Everything, including makefiles.
Now is the time to finally build your libpng, as intended, with configure && make (and all the flags that you've been using so far). And now, it should work.
kloczek
commented
7 months ago Now is the time to finally build your libpng, as intended, with configure && make (and all the flags that you've been using so far). And now, it should work.
And this is what exactly I'm doing. Additionally after successful build test suite execution right after that complains about "make distclean" so seems like it is the test suite issue.
kloczek
commented
7 months ago Gentle ping .. any update? 🤔
Just for the clarification. I'm reusing 100% of the build procedure which has been working correctly with 1.6.40.
kloczek
commented
7 months ago Execution of the "make distclean" after confiture causes that build fails
If you have an existing configured build, then
make distcleanwill clean that up. Everything, including makefiles.Now is the time to finally build your libpng, as intended, with
configure && make(and all the flags that you've been using so far). And now, it should work.
In https://github.com/pnggroup/libpng/issues/544#issuecomment-1925901113 you gave me advise to execute distclean target.
ctruta
commented
7 months ago In #544 (comment) you gave me advise to execute
distcleantarget.
Great! Now that your libpng distribution is clean (hence the name "distclean") it is finally the time to redo the whole build, from the beginning, i.e. from the configure stage.
If you run "make distclean" again, after running "./configure", nothing bad will happen; but nothing good will happen, either... What you have after "make distclean" is not the build, but the clean source tree.
So you run "./configure" and then "make". If you don't specify a target to make, the implicit target will be the libpng build.
kloczek
commented
7 months ago Just retested and found that 1.6.40 is OK and 1.6.41 and 1.6.42 are not. In other words this test suite fails is result of something introduced in 1.6.41.
kloczek
commented
7 months ago Could you please reopen this ticket? 🤔
ctruta
commented
7 months ago Just retested and found that 1.6.40 is OK and 1.6.41 and 1.6.42 are not.
... which means that you went back to square one, in which you had an upgrade from 1.6.40 to 1.6.41 without cleaning the source tree in the process, which is why it had failed for you in the first place.
Just clean everything, then make that clean install of libpng 1.6.42, and that should work.
Could you please reopen this ticket? 🤔
Not from the errors that you are reporting. Please don't mind me asking this question @kloczek but how familiar are you with the configure+make build process? I'm asking because I deduced (from your logs) that you are misusing the configure+make build process during your upgrade. You should do a clean upgrade, and it is apparent to me that you don't.
Moreover, the versions 1.6.41 and 1.6.42 have already been out there for a non-trivial amount of time. Linux distributions picked them up, and so did FreeBSD and Mac, etc. A break-the-world failure like the one that you are reporting would have, umm... broken the world...
ctruta
commented
7 months ago Two more things I would like to add, @kloczek.
First, I would like to clarify that, if you can go back to a clean slate to version 1.6.40 (which worked as expected) and then up to 1.6.41 and then again to 1.6.4.2 (which failed as expected, considering your particular workflow) then you should go back to a clean slate, directly to version 1.6.41. And also, to a clean slate, directly to 1.6.42. And if that wouldn't work, then that would have been a bug. A rather big one.
Second, it is the fact that I do acknowledge your inconvenience, which is why I wrote commit c9ac5bdba40fa59ec85b16100203ead76e4ebf66 to give extra information. Moving on, I also intend to add a build feature in which the configure stage is automatically re-triggered if there is a change of any kind in the file scripts/pnglibconf.dfa. THIS is what has caused your failure, which -- again -- is not a bug, but rather, an expected behaviour. Not an intentional behaviour, mind you, but an expected behaviour nonetheless.
ctruta
commented
7 months ago @jbowler: FYI, see my last comment. We need to re-trigger the pnglibconf step in the configure stage if scripts/pnglibconf.dfa happens to be newer than pnglibconf.h (for example). I will open a separate issue, as a feature request. (Not as a bug report.)
kloczek
commented
7 months ago First, I would like to clarify that, if you can go back to a clean slate to version 1.6.40 (which worked as expected) and then up to 1.6.41 and then again to 1.6.4.2 (which failed as expected, considering your particular workflow) then you should go back to a clean slate, directly to version 1.6.41. And also, to a clean slate, directly to 1.6.42. And if that wouldn't work, then that would have been a bug. A rather big one.
As I wrote first version which fails is 1.6.41 I do not understand what this issue has to do with 1.6.42. Second: could you please tell what EXACTLY needs to be altered in 1.6.40 bud procedure to have fully working procedure on top of >= 1.6.41? 🤔 And please answer on the question: why using dist tarball and just building libpng without any additional operation suddenly 1.6.41 starts failing in that unit of the test suite? 🤔
Yor explanations are not clear and still I font know were is the problem..
ctruta
commented
7 months ago Yor explanations are not clear and still I font know were is the problem..
The simplest explanation that I can give right now is:
You are trying to use a feature named "incremental builds over incremental updates" that doesn't exist at this time in libpng.
Some software programs have it, others don't. We don't. (We should, but we don't.)
We don't actually break the incremental builds of incremental updates on purpose. The explanation of why such breakage happens (for example) between 1.6.40 to 1.6.41, but not (for example) between 1.6.39 and 1.6.40, and also not (for example) between 1.6.41 and 1.6.42, is very technical. Implementing a solution might be as simple as one or two lines of code, but a little insight and a lot of testing is required.
So, @kloczek, this is "Not A Bug", and "Not A Feature", and "Not A Priority" either. Please delete everything you have about 1.6.40 in your local build directory, and start clean from 1.6.41 or 1.6.42, whichever you prefer. That should solve your build problem.
kloczek
commented
7 months ago You are trying to use a feature named "incremental builds over incremental updates" that doesn't exist at this time in libpng.
What you mean "incremental build"? 🤔 As I wrote I'm not doing anything incrementally. Whole build is done as described in in rpm spec file. Here is my libpng.spec
Summary: A library of functions for manipulating PNG image format files
Name: libpng
Epoch: 2
Version: 1.6.40
Release: 4%{?dist}
License: BSD-3-Clause (https://spdx.org/licenses/BSD-3-Clause.html)
URL: http://libpng.org/pub/png/
VCS: https://github.com/glennrp/libpng/
Source: https://sf.net/projects/libpng/files/libpng16/%{version}/%{name}-%{version}.tar.xz
Source: pngusr.dfa
Patch: %{name}-multilib.patch
Patch: %{name}-fix-arm-neon.patch
Patch: %{name}-no_pkgconfig_private.patch
BuildRequires: automake
BuildRequires: gcc
BuildRequires: libtool
BuildRequires: pkgconfig
BuildRequires: pkgconfig(zlib)
%description
The libpng package contains a library of functions for creating and manipulating
PNG (Portable Network Graphics) image format files. PNG is a bit-mapped graphics
format similar to the GIF format. PNG was created to replace the GIF format,
since GIF uses a patented data compression algorithm.
Libpng should be installed if you need to manipulate PNG format image files.
%package devel
Summary: Development files for %{name}
Requires: %{name}%{?_isa} = %{?epoch:%{epoch}:}%{version}-%{release}
Obsoletes: libpng-static
%description devel
This package contains development files for %{name}.
%package tools
Summary: Tools for PNG image format file library
Requires: %{name}%{?_isa} = %{?epoch:%{epoch}:}%{version}-%{release}
%description tools
The libpng-tools package contains tools used by the authors of libpng.
%prep
%autosetup -p1
%__cp %{S:1} .
%build
autoreconf -fiv
%configure \
--disable-static \
--enable-hardware-optimizations \
%{nil}
%make_build DFA_XTRA=pngusr.dfa
%install
%make_install
%check
%make_build check
%files
%{_libdir}/lib*.so.*
%{_mandir}/man5/*
%files devel
%doc libpng-manual.txt example.c
%attr(755,root,root) %{_bindir}/*
%{_includedir}/*
%{_libdir}/lib*.so
%{_libdir}/pkgconfig/*.pc
%{_mandir}/man3/*
%exclude %{_libdir}/lib*.la
%files tools
%attr(755,root,root) %{_bindir}/pngfixPlease show me where is that "incremental build" done. And again you are writing about thing which I've not been asking IGNORING my straight ans as simple as possible questions.
We don't actually break the incremental builds of incremental updates on purpose
censored @#$@ .. I'm not doing any incremental build/updates.
Always is taken as input source OFFICIAL DIST TARBALL.
On building 1.6.41 and 1.6.42 I'm changing ONLY version string in above spec file.
Does it mean that dist tarballs on sf.net are broken? 🤔
kloczek
commented
7 months ago So, @kloczek, this is "Not A Bug", and "Not A Feature", and "Not A Priority" either. Please delete everything you have about 1.6.40 in your local build directory, and start clean from 1.6.41 or 1.6.42, whichever you prefer. That should solve your build problem.
I do not have anything to delete. ALWAYS on building package all is build of of DIST TARBALLS from sf.net. NOITING is reused from prev version build. Is that clear now? 🤔
kloczek
commented
7 months ago You are trying to use a feature named "incremental builds over incremental updates" that doesn't exist at this time in libpng.
I'm not doing any incremental builds/updates. Using rpm it is NOT POSSIBLE to define such build procedure.
ctruta
commented
7 months ago You are trying to use a feature named "incremental builds over incremental updates" that doesn't exist at this time in libpng. I'm not doing any incremental builds/updates. Using rpm it is NOT POSSIBLE to define such build procedure.
I see what you're saying. And yet, if that were truly the case, then you should not have seen the error message asking you to run make distclean after you executed ci_verify_configure.sh. And also, you should not have trouble upgrading from version 1.6.40 to version 1.6.41.
What I am implying here is that something weird goes on in your RPM upgrade, which baffles me. I just looked over the Fedora libpng upgrades, and they haven't upgraded to 1.6.41 or 1.6.42 yet. And neither did OpenSuse, not even Tumbleweed. So maybe the RPM people are stuck in there for some reason, but this is of course, pure conjecture.
I don't know what else to say.
ctruta
commented
7 months ago You are trying to use a feature named "incremental builds over incremental updates" that doesn't exist at this time in libpng. I'm not doing any incremental builds/updates. Using rpm it is NOT POSSIBLE to define such build procedure.
I see what you're saying. And yet, if that were truly the case, then you should not have seen the error message asking you to run
make distcleanafter you executedci_verify_configure.sh. And also, you should not have trouble upgrading from version 1.6.40 to version 1.6.41.What I am implying here is that something weird goes on in your RPM upgrade, which baffles me. I just looked over the Fedora libpng upgrades, and they haven't upgraded to 1.6.41 or 1.6.42 yet. And neither did OpenSuse, not even Tumbleweed. So maybe the RPM people are stuck in there for some reason, but this is of course, pure conjecture.
I don't know what else to say, other than asking you to uninstall, then reinstall, or clean up in any way you can, until this problem gets solved at the RPM's end.
jbowler
commented
7 months ago pngtest -strict pngtest.png fails and the reason is completely explained in the log; it has pretty much zero chance of working with zlib-ng!
Files ./pngtest.png and pngout.png are different
Was ./pngtest.png written with the same maximum IDAT chunk size (8192 bytes),
filtering heuristic (libpng default), compression level (zlib default),
and zlib version (1.3.0.zlib-ng)?This has been happening for ever; in the past changes Mark has made in zlib have made pngtest.out not be byte identical to pngtest.png and pngtest never passed with configured builds where things like the default IDAT size were changed.
Ideally pngtest.png would contain the zlib version info, the IDAT size etc, etc so that this could be checked against the values used by pngtest in the test and disable the complaint if there is a mismatch, but as you said before @ctruta pngtest is way past its "maintain-by" date.
@kloczek just copy the file pngtest.out on top of pngtest.png and I think everything should pass. Since you use zlib-ng you are probably going to have to do this every new release, at least until pngtestgets some active maintenance.
jbowler
commented
7 months ago @ctrtua: this was almost certainly always happening with zlib-ng, however it would only show in cmakebuilds, which just run the one test, in earlier versions of 1.6 because of the bug whereby only the last pngtesttest in configurebuilds was checked. If it keeps showing up just comment out the first test in pngtest-all; not running it is no worse than the previous behavior of ignoring what happened!
jbowler
commented
7 months ago @jbowler: FYI, see my last comment. We need to re-trigger the pnglibconf step in the configure stage if
scripts/pnglibconf.dfahappens to be newer thanpnglibconf.h(for example). I will open a separate issue, as a feature request. (Not as a bug report.)
The dependencies in configure (Makefile.am) are correct so far as I know. Touching pnglibconf.dfa rebuilds pnglibconf.h (at least it did the week before last). There's no magic to messing up a build by untar'ing newer files with older timestamps onto a build tree but I think everyone knowns not to do that! Anyway, it's not what was happening here.
kloczek
commented
7 months ago This has been happening for ever; in the past changes Mark has made in zlib have made pngtest.out not be byte identical to pngtest.png and pngtest never passed with configured builds where things like the default IDAT size were changed.
I'm not using zlib. I'm using zlin-ng.
@ctrtua: this was almost certainly always happening with
zlib-ng, however it would only show incmakebuilds, which just run the one test, in earlier versions of 1.6 because of the bug whereby only the lastpngtesttest inconfigurebuilds was checked. If it keeps showing up just comment out the first test inpngtest-all; not running it is no worse than the previous behavior of ignoring what happened!
Why you are mentioning about cmake? 🤔 Please one more time have look on my libpng.spec file. I'm not using cmake.
Yes, I'm using zlib-ng and again with exactly the same zlib-ng 1.6.40 the test suite is OK, and 1.6.41 and 1.6.42 are NOT.
kloczek
commented
7 months ago The dependencies in
configure(Makefile.am) are correct so far as I know. Touching pnglibconf.dfa rebuildspnglibconf.h(at least it did the week before last). There's no magic to messing up a build by untar'ing newer files with older timestamps onto a build tree but I think everyone knowns not to do that! Anyway, it's not what was happening here.
Please ONE MORE TIME look on top of this ticket on provided output of failing test suite. Unit which is failing name is pngtest-all. That unit reports some diagnostics messages. NONE of those diagnostics messages are related to any .dfa file.
jbowler
commented
7 months ago @kloczek: please try this:
./pngtest --strict pngtest.png pngtest.new
cp pngtest.new pngtest.png
make checkPlease report what happens (whether it succeeds or not).
ctruta
commented
6 months ago After I pushed another bunch of CI commits to further improve our verification process, we can now run those and draw the concluding remarks.
@jbowler, if zlib-ng were at fault, then the test suite would have failed unconditionally. But it didn't. The test suite on the reporter's machine used to work with the older versions, and then it started failing when upgrading from 1.6.40 to 1.6.41, and also, when going back again to 1.6.40 then forward again to the tip-of-tree.
This concludes that zlib-ng is not at fault. My untested assumption is that, due to the changes made that we had made in pnglibconf.dfa about building or not building the Adler32 checks, somehow the obligatory regeneration of pnglibconf.h got skipped. Reading and/or writing the trailing Adler32 might have been messed up inside pngtest.c, I don't know. Or maybe the RPM system did something incorrect, or maybe something else entirely has happened. I really don't know.
@kloczek, after I pushed the last bunch of commits to the repository, including commit 72c4520d3c49cf1db439d835264b21c1aafaa9aa, you should now be able to run a conclusive verification on your own machine, as follows:
CI_MAKE="/usr/bin/make" CI_MAKE_FLAGS="-O -j48 V=1 VERBOSE=1" CI_FORCE=1 ./ci/ci_verify_configure.shPlease ONE MORE TIME look on top of this ticket on provided output of failing test suite. Unit which is failing name is pngtest-all. That unit reports some diagnostics messages. NONE of those diagnostics messages are related to any .dfa file.
With all due respect, @kloczek, the lack of diagnostics related to our DFA files neither proves nor disproves anything, especially considering that these DFA files are actually correct. The lack of diagnostics is something to be expected. I have a feeling that we're going in circles here, each of us repeating each of our past statements but without moving on with the actual investigation of the failure that's been happening on your machine. If you want to help us find out what has been truly going on, then please help us by responding to the last question formulated by @jbowler
Or, alternatively, you can make all failures disappear by wiping out everything in your old libpng repository, and by starting fresh with a fresh git clone at the tip of tree.
kloczek
commented
5 months ago Here is result
kloczek
commented
5 months ago With all due respect, @kloczek, the lack of diagnostics related to our DFA files neither proves nor disproves anything, especially considering that these DFA files are actually correct.
If you have any propositions what I can try to do to diagnose please let me know.
kloczek
commented
5 months ago Summary:
jbowler
commented
5 months ago Please just read the error messages output by pngtest. No diagnosis is required! The error message clearly and unambiguously says why the test is failing and it is failing for precisely the reason I gave on Feb 19. Ok TL;DR - here is a copy-edit of the error log from above. I added the bold emphasis; Glenn was nice enough to ask the relevant questions in addition to clearly stating the problem, just answer the one in bold:
Files /home/tkloczko/rpmbuild/BUILD/libpng-1.6.43/pngtest.png and pngout.png are different Was /home/tkloczko/rpmbuild/BUILD/libpng-1.6.43/pngtest.png written with the same maximum IDAT chunk size (8192 bytes), filtering heuristic (libpng default), compression level (zlib default), and zlib version (1.3.0.zlib-ng)?
The file pngtest.png is distributed with libpng and it is under source control. It was last modified on July 18 2017, this commit: https://github.com/pnggroup/libpng/commit/db16b27d1145b1f9f2f16c79607ba4ca297b2b39
Just check the files! Run pngtestby hand (pngtest pngtest.png) double check that pngout.png is valid; use pngcheckon pngout.png then run pngtopamon both files and diff the results (they should be identical). Now run cmp -b on the two files.
It should be a soft-fail in production but Glenn liked to know if there was a byte-wise change.
kloczek
commented
5 months ago The file pngtest.png is distributed with libpng and it is under source control. It was last modified on July 18 2017, this commit: db16b27
Just check the files! Run
pngtestby hand (pngtest pngtest.png) double check thatpngout.pngis valid; usepngcheckonpngout.pngthen runpngtopamon both files and diff the results (they should be identical). Now runcmp -bon the two files.
I've already presented full source of my build procedure. It has:
Source: https://sf.net/projects/libpng/files/libpng16/%{version}/%{name}-%{version}.tar.xzIf that file is broken it means that IT IS BROKEN IN TAR BALL distributed on sf.net. Have no access now to my system on which I can do that check but you can do that as well. If that file really is broken in dist tar ball it means that it woud be good to release new version.
jbowler
commented
5 months ago This is your build failure:
Files /home/tkloczko/rpmbuild/BUILD/libpng-1.6.43/pngtest.png and pngout.png are different
Most likely nothing is broken. You need to build libpng against zlib-ng on your own system and, as a minimum, attach the pngout.png that pngtest pngtest.png generates to this bug report.
NOTE: not the pngout.png that make check leaves; that one has been overwritten later in the pngtest-all unit test. You have to run pngtest pngtest.png by hand.
jbowler
commented
5 months ago @kloczek @ctruta it is EXACTLY as I said back in February; zlib-ng changes the IDAT. It's 8119 bytes in pngtest.png and 7982 in pngout.png (with 2.1.6).
@kloczek: please read the following page, it may help your understanding of what I said:
https://github.com/zlib-ng/zlib-ng/blob/2.0.x/PORTING.md
In particular it explicitly acknowledges the problem, quoting from the page:
It is also possible for the deflate output stream to differ from the original zlib due to algorithmic differences between the two libraries. Any tests or applications that depend on the exact length of the deflate stream being a certain value will need to be updated.
This is what was previously encountered with zlib version changes and, since zlib-ng is under more active development, the compressed data is quite likely to change with zlib-ng versions. I've attached a pngtest.png which was produced by zlib-ng-2.1.6; it may or may not work with other zlib-ng versions.
@ctruta; the configuration being tested here is "zlib-compat" mode, as described in the README.md above. Gentoo has these words of wisdom:
ewarn "zlib-ng is experimental and replacing the system zlib is dangerous" ewarn "Please be careful!"
I will attach a "zlib.h" which handles the "native mode" name changes sufficiently for libpng; just drop it in the source directory (it might work in the build directory too, but I didn't try that.) configure must link -lz-ng too; the following is more than sufficient:
CFLAGS='-Wall -Wextra' LIBS="-lz-ng -lm" ./configure -enable-werror
Note that this gets the version number right too (unlike the logs from compat mode).
As I said before this error should be XFAIL'ed but if you intend to replace pngtest with unit tests the work to make that happen can't be justified.
zlib.h is attached as zlib.h.txt - stupid github: zlib.h.txt
kloczek
commented
5 months ago @kloczek @ctruta it is EXACTLY as I said back in February; zlib-ng changes the IDAT. It's 8119 bytes in pngtest.png and 7982 in pngout.png (with 2.1.6).
Hmm .. if that file needs to be generated before start test suite why this file is not build during the build? 🤔
jbowler
commented
5 months ago @kloczek @ctruta it is EXACTLY as I said back in February; zlib-ng changes the IDAT. It's 8119 bytes in pngtest.png and 7982 in pngout.png (with 2.1.6).
Hmm .. if that file needs to be generated before start test suite why this file is not build during the build? 🤔
zlib-ng is not completely compatible with zlib therefore the test fails. This is the intended behavior.
kloczek
commented
5 months ago zlib-ng is not completely compatible with zlib therefore the test fails. This is the intended behavior.
I understand that however more and more distros are using only zlib-ng so IMO that situation needs to be handel IMO somehow automatically 🤔
jbowler
commented
5 months ago I understand that however more and more distros are using only zlib-ng so IMO that situation needs to be handel IMO somehow automatically 🤔
I agree that a patch to, as a minimum, accommodate zlib-ng would be worthwhile. I think you just volunteered to do that, unless @ctruta has an alternative?
Looks like with new version test suite started failing