Skip to content

build for 3.9.0#32

Merged
github-actions[bot] merged 15 commits into
conda-forge:masterfrom
h-vetinari:master
Oct 18, 2020
Merged

build for 3.9.0#32
github-actions[bot] merged 15 commits into
conda-forge:masterfrom
h-vetinari:master

Conversation

@h-vetinari

@h-vetinari h-vetinari commented Mar 3, 2020

Copy link
Copy Markdown
Member

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Note: url for sources changed between 3.8.0 and 3.9.0.

Edit: Fix #33.

@conda-forge-linter

Copy link
Copy Markdown

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:

  • It looks like the 'libblas' output doesn't have any tests.
  • It looks like the 'libcblas' output doesn't have any tests.
  • It looks like the 'liblapack' output doesn't have any tests.
  • It looks like the 'liblapacke' output doesn't have any tests.
  • It looks like the 'blas-devel' output doesn't have any tests.

@h-vetinari

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown

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:

  • It looks like the 'libblas' output doesn't have any tests.
  • It looks like the 'libcblas' output doesn't have any tests.
  • It looks like the 'liblapack' output doesn't have any tests.
  • It looks like the 'liblapacke' output doesn't have any tests.
  • It looks like the 'blas-devel' output doesn't have any tests.
  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

@h-vetinari

Copy link
Copy Markdown
Member Author

@conda-forge-admin, please rerender

@h-vetinari

h-vetinari commented Apr 24, 2020

Copy link
Copy Markdown
Member Author

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

h-vetinari commented Apr 24, 2020

Copy link
Copy Markdown
Member Author

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

hadim commented Apr 24, 2020

Copy link
Copy Markdown
Member

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

@h-vetinari

Copy link
Copy Markdown
Member Author

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

Sure, I can do that.

h-vetinari added a commit to h-vetinari/lapack-feedstock that referenced this pull request Apr 25, 2020
@conda-forge-linter

Copy link
Copy Markdown

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:

  • It looks like the 'libblas' output doesn't have any tests.
  • It looks like the 'libcblas' output doesn't have any tests.
  • It looks like the 'liblapack' output doesn't have any tests.
  • It looks like the 'liblapacke' output doesn't have any tests.
  • It looks like the 'blas-devel' output doesn't have any tests.

@h-vetinari

Copy link
Copy Markdown
Member Author

@conda-forge-admin, please rerender

@h-vetinari

h-vetinari commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

@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 Reference-LAPACK/lapack#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.

Comment thread recipe/patches/upstream-PR-367.patch Outdated
@h-vetinari

Copy link
Copy Markdown
Member Author

@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

isuruf commented Apr 25, 2020

Copy link
Copy Markdown
Member

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

@h-vetinari

h-vetinari commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

@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 h-vetinari force-pushed the master branch 6 times, most recently from 1e72361 to ac718ef Compare April 25, 2020 21:43
@h-vetinari

h-vetinari commented Apr 25, 2020

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Member Author

Ping @jjhelmus, @mbargull

@h-vetinari

Copy link
Copy Markdown
Member Author

@conda-forge-admin, please rerender

@h-vetinari

h-vetinari commented Sep 4, 2020

Copy link
Copy Markdown
Member Author

@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?

h-vetinari and others added 9 commits September 6, 2020 23:19
Turn .diff into actual patch and rebase on 3.9.0; move to /patches.
Also remove unused 4-year-old patch
Co-Authored-By: Isuru Fernando <isuruf@gmail.com>
Since `{{ version }}` goes down to patch level, this doesn't changes
anything, but without it, there are warnings like:

    Adding .* to spec 'libblas 3.9.0' to ensure satisfiability. Please consider putting
    {{ var_name }}.* or some relational operator (>/</>=/<=) on this spec in meta.yaml,
    or if req is also a build req, using {{ pin_compatible() }} jinja2 function instead.
    See https://conda.io/docs/user-guide/tasks/build-packages/variants.html#pinning-at-the-variant-level
Also includes some fixes for regressions, bugs & undefined behaviour
that are being carried by the OpenBLAS project.
@isuruf isuruf added the automerge Merge the PR when CI passes label Oct 18, 2020
@github-actions

github-actions Bot commented Oct 18, 2020

Copy link
Copy Markdown
Contributor

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

I considered the following status checks when analyzing this PR:

  • linter: passed
  • drone: passed
  • travis: passed
  • azure: passed

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

@github-actions github-actions Bot merged commit 40f8bc3 into conda-forge:master Oct 18, 2020
isuruf added a commit that referenced this pull request Oct 22, 2020
Carry some upstream patches (that were pending for #32)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Merge the PR when CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"lapack" package should be removed

4 participants