Dear Lumin, Le dimanche 28 avril 2019 à 09:36 +0000, Mo Zhou a écrit : > Please start checking the updated openblas packaging here, I'm almost > finalizing the changes. The experimental package can be built and > installed: > https://salsa.debian.org/science-team/openblas/tree/lumin > > > I say "almost" because this is a hell of detail. I still have to > check > the package contents again, and compare the headers for different > variants. That would still take some time but the basic shaping of > the package was finished. I guess I still have to fix some missing > "64" postfixes, or something alike. I took some time reviewing it (only looking at source code for the time being, I haven’t yet compiled or tested it). Here are my comments. The first is a major issue, the others are minor or easy to address. 1) Unless I am missing something, I think the LAPACK provided by the 64-bit indexing variants is broken. The problem is that OpenBLAS provides optimized versions of only a subset of the LAPACK routines. The others are directly copied from the liblapack-pic binary, which is has 32-bit integer indexing. Hence the generated 64-bit indexing LAPACK is a mixture of 32-bit and 64-bit indexing routines, depending on whether OpenBLAS overrides them, which is obviously broken. I think there is no other choice but to build a 64-bit indexing variant of src:lapack (either just providing a liblapack64-pic, providing the full-fledged alternatives; I tend to favor the second option, but I understand that you're not interested in that; this can probably be discussed in a separate thread). 2) You removed the "-frecursive" Fortran option in debian/rules. I think this is incorrect. This option is added by Makefile.system. It is necessary for correct operation of LAPACK (see #693269; see also the debian/rules of src:lapack). I understand from your comments that the option creates a test failure somewhere, so more investigation is probably needed. 3) libopenblas-base should be in section “oldlibs”, since it is a transitional package 4) There were two preinst scripts (one for the shared library, one for the dev package) whose purpose was to remove old alternatives from the pre-multiarch layout, during the stretch→buster upgrade. You moved these scripts to the new binary packages, but I think this is incorrect, since these scripts are only needed when upgrading from older packages. I think these scripts should either be left as they were (and no preinst should be added to the 64-bit flavour), or they could as well be entirely dropped, since they won’t be needed for the buster→bullseye upgrade. 5) Maybe we should put more space between priority levels. Instead of 100>99>98, maybe put 100>95>90. So that it is easier to put something in between, should the need arise. 6) In debian/rules, the test for determining whether the architecture is 64-bit could probably be simplified and made more generic by checking the value of the variable DEB_HOST_ARCH_BITS. 7) The extended package descriptions are the same across variants. I think they should be differentiated by explaining the options contained in a given package (and in particular, mention that 32-bit indexing vs 64-bit indexing is just about matrix indexing, not about CPU ISA). 8) The git history is quite messy. I think your branch should be squashed in a single commit. Thanks for your work, I think it’ll be a great step forward for t he OpenBLAS package (once we’ve fixed these issues). Best, -- ⢀⣴⠾⠻⢶⣦⠀ Sébastien Villemot ⣾⠁⢠⠒⠀⣿⡁ Debian Developer ⢿⡄⠘⠷⠚⠋⠀ http://sebastien.villemot.name ⠈⠳⣄⠀⠀⠀⠀ http://www.debian.org
Attachment:
signature.asc
Description: This is a digitally signed message part