The package media-libs/libsndfile does currently not support multilib ABIs. It is instead provided by the app-emulation/emul-linux-x86-soundlibs. It would be nice if it could be dropped from the emul-package and instead support multilib through the autotools-multilib eclass. I will submit an ebuild that has multilib ABI support. I will also attach the diff just for the overview. Some major things that might be of interest are: 1. The fix for #313259 has been changed and the package should thus not be bundled in an emul-package. It does not need to anyways. 2. The source has to be patched to honour the --htmldir option to configure. The patch has already been accepted upstream [1]. I will attach the patch. 3. dev-db/sqlite does not yet support multilib ABIs so libsndfile has to depend on app-emulation/emul-linux-x86-baselibs. 4. epunt_cxx has been removed accepting the attached ebuild will fix #458294 [1] https://github.com/erikd/libsndfile/commit/315c92457b1edbea2c9bf55fe57e37ad17a7a086
Created attachment 346132 [details] The proposed ebuild with multilib ABI support.
Created attachment 346134 [details, diff] The diff between the proposed ebuild and the one in tree.
Created attachment 346136 [details, diff] The patch to honour htmldir.
Created attachment 346138 [details] The new proposed ebuild with multilib ABI support. Oops, it should block <=app-emulation/emul-linux-x86-soundlibs-20130224 if abi_x86_32 is set.
Created attachment 346140 [details, diff] The diff between the proposed ebuild and the one in tree.
-MY_P=${P/_pre/pre} ... -if [[ "${MY_P}" == "${P}" ]]; then - SRC_URI="http://www.mega-nerd.com/libsndfile/files/${P}.tar.gz" -else - SRC_URI="http://www.mega-nerd.com/tmp/${MY_P}b.tar.gz" -fi +SRC_URI="http://www.mega-nerd.com/${PN}/files/${P}.tar.gz" ... -S=${WORKDIR}/${MY_P} This was there for a reason :) + sqlite? ( + >=dev-db/sqlite-3.2 + amd64? ( abi_x86_32? ( app-emulation/emul-linux-x86-baselibs ) ) I'd rather wait for sqlite to be converted (do the leaves first! :) ) - AT_M4DIR=M4 eautoreconf - epunt_cxx + export AT_M4DIR=M4 + autotools-multilib_src_prepare you dont need the 'export' if you put it on the same line - multilib_enabled && append-lfs-flags #313259 + use amd64 && use abi_x86_32 && append-lfs-flags #313259 not sure at all about this, sndfile.h is generated and has in its .in: typedef @TYPEOF_SF_COUNT_T@ sf_count_t ; #define SF_COUNT_MAX @SF_COUNT_MAX@ I assume you are doing this because the header checks were failing; if unconditionnally calling 'append-lfs-flags' works then I'd say go for it. 'use amd64 && use abi_x86_32 && append-lfs-flags' is likely wrong anyway.
(In reply to comment #6) > - multilib_enabled && append-lfs-flags #313259 > + use amd64 && use abi_x86_32 && append-lfs-flags #313259 > > not sure at all about this, sndfile.h is generated and has in its .in: > typedef @TYPEOF_SF_COUNT_T@ sf_count_t ; > #define SF_COUNT_MAX @SF_COUNT_MAX@ > > > I assume you are doing this because the header checks were failing; if > unconditionnally calling 'append-lfs-flags' works then I'd say go for it. > 'use amd64 && use abi_x86_32 && append-lfs-flags' is likely wrong anyway. Hmm, don't append-lfs-flags unconditionnally: see bug #335728 ; maybe wrapping this header could be the solution.
(In reply to comment #6) > -MY_P=${P/_pre/pre} > ... > -if [[ "${MY_P}" == "${P}" ]]; then > - SRC_URI="http://www.mega-nerd.com/libsndfile/files/${P}.tar.gz" > -else > - SRC_URI="http://www.mega-nerd.com/tmp/${MY_P}b.tar.gz" > -fi > +SRC_URI="http://www.mega-nerd.com/${PN}/files/${P}.tar.gz" > ... > -S=${WORKDIR}/${MY_P} > > This was there for a reason :) > Oh, sorry! I didn't seem to be needed and removing it appeared cleaner to me, but please keep it if you want! :) > > + sqlite? ( > + >=dev-db/sqlite-3.2 > + amd64? ( abi_x86_32? ( app-emulation/emul-linux-x86-baselibs ) ) > > I'd rather wait for sqlite to be converted (do the leaves first! :) ) > The only issue I see here is that sqlite is part of emul-linux-x86-baselibs which means it will block baselibs and baselibs is pulled in by some other packages. That would make it impossible to install this (pretty important) library. Therefore I would like if it depends on baselibs for now and gets something like this later on: amd64? ( abi_x86_32? ( || ( app-emulation/emul-linux-x86-baselibs >=dev-libs/sqlite[${MULTILIB_USEDEP}] ) ) ) That approach would make the package installable even for people keeping baselibs for some reason and would make the transition to multilib based ebuilds smoother. That is at least my opinion. > - AT_M4DIR=M4 eautoreconf > - epunt_cxx > + export AT_M4DIR=M4 > + autotools-multilib_src_prepare > > you dont need the 'export' if you put it on the same line > I didn't know how autotools-multilib_src_prepare would handle the argument if it was to be passed on the same line so I took the solution which I know works. Is there some bad things about exporting? > - multilib_enabled && append-lfs-flags #313259 > + use amd64 && use abi_x86_32 && append-lfs-flags #313259 > > not sure at all about this, sndfile.h is generated and has in its .in: > typedef @TYPEOF_SF_COUNT_T@ sf_count_t ; > #define SF_COUNT_MAX @SF_COUNT_MAX@ > > > I assume you are doing this because the header checks were failing; if > unconditionnally calling 'append-lfs-flags' works then I'd say go for it. > 'use amd64 && use abi_x86_32 && append-lfs-flags' is likely wrong anyway. Correct assumption.
@aballier, wouldn't appending LFS flags unconditionally break binary compatibility without changing SONAME on arches where it wasn't enabled previously? I'd rather wait with changing that until SONAME changes upstream.
Looking at bug #313259 and thinking about it more, I don't think it was the correct thing to do. It was a hack to avoid header wrapping for the specific need of building emul-linux. I'm CC-ing amd64@ as the maintainers of emul-linux. Now we can: 1. carry the hack on when building 32-bit version on multilib (ultra-hacky!) - keeps ABI intact with emul-linux, - our lib is potentially incompatible with upstream binary packages, - native 32-bit libsndfile differs from multilib 32-bit libsndfile. 2. drop the hack and wrap header properly - more consistent and compatible, - requires header wrapping, - ABI change -- how to handle it without inventing our own SONAME?
I would go to wrapping the header
(In reply to comment #11) > I would go to wrapping the header What about the ABI change? I'm afraid this will be a case which would require users to manually rebuild the package. Even revdep-rebuild won't catch the failure. Of course, the other question is whether there will be any actual failing consumers. I'd expect the only pieces linking to the library would be ones within emul-linux.
Taking care emul packages should go away and that it was, effectively, the incorrect thing to do, I would still drop the hack. I think there aren't many packages linking to libsndfile from emul set :/
(In reply to comment #13) > Taking care emul packages should go away and that it was, effectively, the > incorrect thing to do, I would still drop the hack. I think there aren't > many packages linking to libsndfile from emul set :/ Curious enough, the tests fail without LFS with abi_x86_32. Even more curious, they fail even if abi_x86_64 is not built and sndfile is not installed. G72x/g72x_test all g723_test : Max error of 585 at postion 3013. ok ./test_main Testing big endian conversions : Line 98 : 64 bit int failed 0x123456789abcdefx -> 0x89abcdefx. Could someone tell me if the tests pass native x86?
Created attachment 347332 [details] Updated ebuild (the ebuild with test failure for abi_x86_32) @Karl, thanks once again for your contributions. A few more notes though: 1. please try not to change developer's coding style. EAPI="5" is correct, though the quoting is unnecessary and the ebuild didn't quote it before. 2. i think we can hack docdir the 'shorter' way rather than applying the patch. the next version won't need it anyway.
(In reply to comment #15) > Created attachment 347332 [details] > Updated ebuild > > (the ebuild with test failure for abi_x86_32) > > @Karl, thanks once again for your contributions. A few more notes though: > > 1. please try not to change developer's coding style. EAPI="5" is correct, > though the quoting is unnecessary and the ebuild didn't quote it before. > > 2. i think we can hack docdir the 'shorter' way rather than applying the > patch. the next version won't need it anyway. You're welcome. OK, I understand. Please do so.
Oh, someone with x86 please also test the following: $ sndfile-convert a.wav a.rf64 (supplying a random a.wav or any other supported input) without LFS, it fails with an assertion (lack of 64-bit types support, I suppose). Not that the library should base it on top of LFS...
I don't have just now a x86 machine to test, but looking to fedora .spec seems that they apply some changes to the header for multilib (lines 83 to 103): http://pkgs.fedoraproject.org/cgit/libsndfile.git/tree/libsndfile.spec And also run tests differently (probably to use just compiled files instead of system ones) -> line 111
(In reply to comment #18) > I don't have just now a x86 machine to test, but looking to fedora .spec > seems that they apply some changes to the header for multilib (lines 83 to > 103): > http://pkgs.fedoraproject.org/cgit/libsndfile.git/tree/libsndfile.spec That seems like our header wrapping. > And also run tests differently (probably to use just compiled files instead > of system ones) -> line 111 Just tried that and doesn't seem to help. And while at it, I also removed the system libs and checked, and the package seems to be able to find just-compiled copy. Maybe their .spec is outdated.
Ok, I've just tested on systemrescuecd and the package seems to work fine on x86. Looking at the code, they're using some custom largefile support macros. Those macros work fine on x86 but fail on amd64 with -m32. Then, appending the LFS was correct -- but we need to investigate why the build system fails to do that on amd64.
Created attachment 347342 [details] Ebuild with LFS flags
Created attachment 347348 [details] Upstream LFS fix + python improvement Ok, one more try. This time LFS is done per upstream commit -- they've replaced the broken custom macro with a standard one, and since we're doing an autoreconf anyway, we can as well patch configure.ac. I've also added Python deps for test phase (bug #395817). Please review this one and lemme know if I can commit it.
Committed as 1.0.25-r1, pmasked.
