search

conda-forge / lapack-feedstock

A conda-smithy repository for lapack.
BSD 3-Clause "New" or "Revised" License
6 stars 12 forks source link

build for 3.9.0 #32

Closed h-vetinari closed 4 years ago

h-vetinari commented 4 years ago

Checklist

Note: url for sources changed between 3.8.0 and 3.9.0.

Edit: Fix #33.

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

h-vetinari commented 4 years ago

OK, this is failing with undefined symbols, e.g. OSX:

[100%] Linking C shared library ../lib/liblapacke.dylib
ld: warning: -pie being ignored. It is only used when linking a main executable
Undefined symbols for architecture x86_64:
  "_LAPACK_cgeqpf", referenced from:
      _LAPACKE_cgeqpf_work in lapacke_cgeqpf_work.c.o
  "_LAPACK_cggsvd", referenced from:
      _LAPACKE_cggsvd_work in lapacke_cggsvd_work.c.o
  "_LAPACK_cggsvp", referenced from:
      _LAPACKE_cggsvp_work in lapacke_cggsvp_work.c.o
  "_LAPACK_dgeqpf", referenced from:
      _LAPACKE_dgeqpf_work in lapacke_dgeqpf_work.c.o
  "_LAPACK_dggsvd", referenced from:
      _LAPACKE_dggsvd_work in lapacke_dggsvd_work.c.o
  "_LAPACK_dggsvp", referenced from:
      _LAPACKE_dggsvp_work in lapacke_dggsvp_work.c.o
  "_LAPACK_sgeqpf", referenced from:
      _LAPACKE_sgeqpf_work in lapacke_sgeqpf_work.c.o
  "_LAPACK_sggsvd", referenced from:
      _LAPACKE_sggsvd_work in lapacke_sggsvd_work.c.o
  "_LAPACK_sggsvp", referenced from:
      _LAPACKE_sggsvp_work in lapacke_sggsvp_work.c.o
  "_LAPACK_zgeqpf", referenced from:
      _LAPACKE_zgeqpf_work in lapacke_zgeqpf_work.c.o
  "_LAPACK_zggsvd", referenced from:
      _LAPACKE_zggsvd_work in lapacke_zggsvd_work.c.o
  "_LAPACK_zggsvp", referenced from:
      _LAPACKE_zggsvp_work in lapacke_zggsvp_work.c.o
ld: symbol(s) not found for architecture x86_64

There are a couple of patches upstream that address build errors (openblas also carried another bugfix, for example), maybe it'll be necessary to do that here as well?

Windows OTOH errors even earlier

ECHO "Did not find VS in registry or in VS90COMNTOOLS env var - exiting"
conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

h-vetinari commented 4 years ago

@conda-forge-admin, please rerender

h-vetinari commented 4 years ago

Alright, with the switch to vs2017, windows is now building at least. :)

It fails with 

105/109 Test #105: LAPACK_Test_Summary ..............   Passed    0.46 sec
        Start 106: example_DGESV_rowmajor
Unable to find executable: D:/bld/blas-split_1587736997740/work/build/bin/xexample_DGESV_rowmajor
Could not find executable D:/bld/blas-split_1587736997740/work/build/bin/xexample_DGESV_rowmajor

which looks to be related to the errors when building on linux/osx:

[100%] Linking C executable ../../bin/xexample_DGELS_rowmajor
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_cggsvp'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_dgeqpf'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_zggsvp'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_sggsvd'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_zggsvd'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_dggsvd'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_zgeqpf'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_sggsvp'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_cggsvd'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_cgeqpf'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_sgeqpf'
[...snip...]/ld: ../../lib/liblapacke.so.3.9.0: undefined reference to `LAPACK_dggsvp'
collect2: error: ld returned 1 exit status

Looking into this a bit further, linux seems to be giving the clearest errors as to why that's happening. Seems the functions are only defined implicitly (note the different logs seem to be racy; the warning follows 1-2 lines too late):

[ 98%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cggsvp.c.o
[ 98%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cggsvp_work.c.o
[ 98%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_dggsvp.c.o
[...snip...]/LAPACKE/src/lapacke_cggsvp_work.c: In function 'LAPACKE_cggsvp_work':
[...snip...]/LAPACKE/src/lapacke_cggsvp_work.c:52:9: warning: implicit declaration of function 'LAPACK_cggsvp'; did you mean 'LAPACKE_cggsvp'? [-Wimplicit-function-declaration]
         LAPACK_cggsvp( &jobu, &jobv, &jobq, &m, &p, &n, a, &lda, b, &ldb, &tola,
         ^~~~~~~~~~~~~
         LAPACKE_cggsvp
[ 98%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_dggsvp_work.c.o
[ 98%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_sggsvp.c.o
[...snip...]/LAPACKE/src/lapacke_dggsvp_work.c: In function 'LAPACKE_dggsvp_work':
[...snip...]/LAPACKE/src/lapacke_dggsvp_work.c:48:9: warning: implicit declaration of function 'LAPACK_dggsvp'; did you mean 'LAPACKE_dggsvp'? [-Wimplicit-function-declaration]
         LAPACK_dggsvp( &jobu, &jobv, &jobq, &m, &p, &n, a, &lda, b, &ldb, &tola,
         ^~~~~~~~~~~~~
         LAPACKE_dggsvp

... and so on for the other functions that error at link time.

h-vetinari commented 4 years ago

I looked into the functions upstream, and first found that all the function where the linker struggles have been set as deprecated in CMakeLists.txt.

While both build.sh and build.bat are passing DBUILD_DEPRECATED=ON (and the corresponding lapack-functions get built), they aren't found from within lapacke (e.g. here). Maybe these are somehow not correctly inserted into lapack.h?

That's about as far as I got now... @conda-forge/lapack

hadim commented 4 years ago

@h-vetinari do you mind removing me as a maintainer?

h-vetinari commented 4 years ago

@hadim: @h-vetinari do you mind removing me as a maintainer?

Sure, I can do that.

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

h-vetinari commented 4 years ago

@conda-forge-admin, please rerender

h-vetinari commented 4 years ago

@h-vetinari: Maybe these [deprecated functions] are somehow not correctly inserted into lapack.h?

This was the right conclusion - it has been fixed upstream in https://github.com/Reference-LAPACK/lapack/pull/367, but it doesn't look like they'll release 3.9.1 anytime soon, so I'm adding the patch.

While I was at it, I also added a (cleaned-up) patch I saw being carried by openblas, and cleaned up the existing patches/diffs.

h-vetinari commented 4 years ago

@isuruf Well that made a right big mess. ~700 lines of error for just one function!

If there's something that I can make out, it's that the header function signature doesn't match the callsite signature, and we get errors like

warning: passing argument 1 of 'cggsvp_' makes integer from pointer without a cast [-Wint-conversion]
note: expected 'int' but argument is of type 'char *'
warning: passing argument 2 of 'cggsvp_' makes integer from pointer without a cast [-Wint-conversion]
note: expected 'char' but argument is of type 'char *'

etc.

isuruf commented 4 years ago

Let's just go with the upstream patch. Sorry about the mess.

h-vetinari commented 4 years ago

@isuruf: Let's just go with the upstream patch. Sorry about the mess.

No, I think you're on to something. And besides, even with that patch the windows build fails

I've got an experimental patch that should fix the functions in question. There's an actual signature difference. Let's see.

h-vetinari commented 4 years ago

Since azure is gonna throw the logs away again (although let's see if the permalinks work; edit: nope), I'm recording another build error here on windows.

[ 59%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cgesvdq.c.obj
D:\bld\blas-split_1587851181980\work\LAPACKE\src\lapacke_cgesvdq.c: In function 'LAPACKE_cgesvdq':
D:\bld\blas-split_1587851181980\work\LAPACKE\src\lapacke_cgesvdq.c:69:34: warning: passing argument 21 of 'LAPACKE_cgesvdq_work' from incompatible pointer type [-Wincompatible-pointer-types]
                                  &rwork_query, lrwork );
                                  ^
In file included from D:/bld/blas-split_1587851181980/work/LAPACKE/include/lapacke_utils.h:37:0,
                 from D:\bld\blas-split_1587851181980\work\LAPACKE\src\lapacke_cgesvdq.c:34:
D:/bld/blas-split_1587851181980/work/LAPACKE/include/lapacke.h:5769:12: note: expected 'float *' but argument is of type 'double *'
 lapack_int LAPACKE_cgesvdq_work( int matrix_layout, char joba, char jobp,
            ^
D:\bld\blas-split_1587851181980\work\LAPACKE\src\lapacke_cgesvdq.c:74:5: error: aggregate value used where an integer was expected
     lcwork = (lapack_int)cwork_query;
     ^
D:\bld\blas-split_1587851181980\work\LAPACKE\src\lapacke_cgesvdq.c:95:64: warning: passing argument 21 of 'LAPACKE_cgesvdq_work' from incompatible pointer type [-Wincompatible-pointer-types]
                                  iwork, liwork, cwork, lcwork, rwork, lrwork );
                                                                ^
In file included from D:/bld/blas-split_1587851181980/work/LAPACKE/include/lapacke_utils.h:37:0,
                 from D:\bld\blas-split_1587851181980\work\LAPACKE\src\lapacke_cgesvdq.c:34:
D:/bld/blas-split_1587851181980/work/LAPACKE/include/lapacke.h:5769:12: note: expected 'float *' but argument is of type 'double *'
 lapack_int LAPACKE_cgesvdq_work( int matrix_layout, char joba, char jobp,
            ^
mingw32-make[2]: *** [LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cgesvdq.c.obj] Error 1
LAPACKE\CMakeFiles\lapacke.dir\build.make:17159: recipe for target 'LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cgesvdq.c.obj' failed

This happens on linux/osx as well, but there it's just a warning:

[ 79%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cgesvdq.c.o
/usr/local/miniconda/conda-bld/blas-split_1587851132180/work/LAPACKE/src/lapacke_cgesvdq.c:69:34: warning: incompatible pointer types passing 'double *' to parameter of type 'float *' [-Wincompatible-pointer-types]
                                 &rwork_query, lrwork );
                                 ^~~~~~~~~~~~
/usr/local/miniconda/conda-bld/blas-split_1587851132180/work/LAPACKE/include/lapacke.h:5778:40: note: passing argument to parameter 'rwork' here
                                float* rwork, lapack_int lrwork);
                                       ^
/usr/local/miniconda/conda-bld/blas-split_1587851132180/work/LAPACKE/src/lapacke_cgesvdq.c:95:64: warning: incompatible pointer types passing 'double *' to parameter of type 'float *' [-Wincompatible-pointer-types]
                                 iwork, liwork, cwork, lcwork, rwork, lrwork );
                                                               ^~~~~
/usr/local/miniconda/conda-bld/blas-split_1587851132180/work/LAPACKE/include/lapacke.h:5778:40: note: passing argument to parameter 'rwork' here
                                float* rwork, lapack_int lrwork);
                                       ^
2 warnings generated.
[ 79%] Building C object LAPACKE/CMakeFiles/lapacke.dir/src/lapacke_cgesvdq_work.c.o
h-vetinari commented 4 years ago

Phew, this one was a tough nut to crack 😅

The last commit is not absolutely necessary (the test suite passes without it), but it just seems wrong on several levels, and the compiler warnings make it sound like it has no idea which function to use. Not sure if the end result is actually tested (since the functions are deprecated), or would be working without this. I'd propose to keep it.

On windows at least, those warnings were misleading however, as they made me chase off in the wrong direction compared to the actual build errors that were happening.

In any case, I'll be trying to upstream those 3 patches.

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

h-vetinari commented 4 years ago

I thought I might as well fix #33 while I'm at it. Following @isuruf's suggestion from this comment.

h-vetinari commented 4 years ago

While preparing the patches for upstreaming, I discovered that part of the fixes I did were already done in another PR upstream. Retrying to see how things go with that upstream patch.

h-vetinari commented 4 years ago

So, aarch64/ppc64le aren't ready to have LAPACK-xeigtstz* enabled again, unfortunately; I removed the patch.

I also ran into build errors (should have saved the log) when trying to enable the more_testing-patch from the blas-feedstock that also runs the BLAS/LAPACKE tests, but I'm guessing those are included here already.

h-vetinari commented 4 years ago

OK @isuruf, this should finally be ready from my side. I've opened a PR to upstream the additional commits in https://github.com/Reference-LAPACK/lapack/pull/409, let's see how that goes.

h-vetinari commented 4 years ago

So, one of the reasons that I overlooked the build errors on windows initially, is because I was fooled by specific test errors. However, the test suite shouldn't even have run after the build already failed.

Hence https://github.com/conda-forge/lapack-feedstock/pull/32/commits/cf4ee5993baadbfd89e7869ef367a10b144c04fb, to make sure we don't run the test suite if the build already failed.

h-vetinari commented 4 years ago

@isuruf Seeing that your last review was only a nit, I'm hoping this is ready to go?

Or do you want to wait for a response upstream?

isuruf commented 4 years ago

I don't think we can merge it just yet. MKL doesn't yet support 3.9.0 and see the below issue. https://github.com/conda-forge/intel_repack-feedstock/pull/5#issuecomment-619411254

@jjhelmus, @mbargull, this netlib variant has a track_features entry so that it is not installed by default and only the openblas variant is installed by default. If I merge this with a new version, I assume only the netlib variant will be in current_repodata.json. Does that mean, the netlib variant will be installed?

h-vetinari commented 4 years ago

Cool, thanks for the context, i just didn't know what's outstanding. MKL will probably take a while?

If things work with track_features that would be great. The other build variants in the blas-recipe have this enabled as well.

h-vetinari commented 4 years ago

@jjhelmus, @mbargull Can you comment on the track_features vs blas-variants question from @isuruf? MKL does not yet have lapack 3.9 support, but netlib/openblas should be ready (I didn't find compat info for blis, but the last release is only 3 weeks ago, so probably ok)

h-vetinari commented 4 years ago

Ping @jjhelmus, @mbargull

h-vetinari commented 4 years ago

@conda-forge-admin, please rerender

h-vetinari commented 4 years ago

@isuruf The patches developed in the course of this PR have now been fully upstreamed to Reference-LAPACK.

@jjhelmus @mbargull, any comment on the following?

@isuruf: @jjhelmus, @mbargull, this netlib variant has a track_features entry so that it is not installed by default and only the openblas variant is installed by default. If I merge this with a new version, I assume only the netlib variant will be in current_repodata.json. Does that mean, the netlib variant will be installed?

github-actions[bot] commented 4 years ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!