Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few remaining issues with CMake build and git (SDK-26) #2

Merged
merged 2 commits into from
Feb 10, 2016

Conversation

bartlettroscoe
Copy link
Contributor

Sherry,

I found a few issues with the current 'master' branch Two of the issues mentioned below are fixed by commits in the PR:

  1. The generated file make.inc is no longer being ignored. (This is fixed in the new PR.)

  2. The built-in CBLAS is not on by default and ignores TPL_BLAS_LIBRARIES. That is incompatible with the XSDK configure standard and kills library interoperability. When USE_XSDK_DEFAULTS=ON, it must not use the use the built-in CBLAS by default. (This is fixed in the new PR.)

I was testing the internal BLAS by turning ON, but forgot to change back to OFF.

  1. There is no auotmated testing for the user example at all, even for the narrow use cases where it currently works. That is asking for the examples to be broken when users try to use it. (I did not fix this but this is not an issues for xSDK.)

The TESTING/ code is meant for automated testing. Once they pass, the EXAMPLE/ code should work. The examples are convenient for users to stick in their application codes. The EXAMPLE/Makefile needs to include ../make.inc, so the users code can easily catch the compiler/flags/libraries-built. The problem i have now is I don't know how to specify libsuperlu.a / libsuperlu.so, nor libblas.a / libblas.so (if using CBLAS) in make.inc.in configure file. Can you help with that?

Can you please just accept my new pull request? This is just two simple commits fixing the issues described above. That way I will get some credit for contributing to SuperLU.

I just merged your 2 changes. Yesterday I was going through each of them, just be sure I understand the issues ... Now I know how this can be done easily.

-Ross

Roscoe A. Bartlett added 2 commits February 10, 2016 11:34
You have to ignore this file or get will constantly complain about this file
being untracked!
By default, this was ignoring TPL_BLAS_LIBRARIES which violates the xSDK
configure standard.  This is obviously bad because this would destroy
interoperability with other packages that also use BLAS (since there would be
two definitions of all the BAS rountimes).

SuperLU can do whatever it wants with its default non-xSDK configure but not
when USE_XSDK_DEFAULTS=ON is set.
xiaoyeli added a commit that referenced this pull request Feb 10, 2016
Fix a few remaining issues with CMake build and git (SDK-26)
@xiaoyeli xiaoyeli merged commit d20e15c into xiaoyeli:master Feb 10, 2016
@bartlettroscoe
Copy link
Contributor Author

Thanks for merging this! Hopefully this review was helpful. I think the issues with the examples can be fixed without too much trouble.

-Ross

@xiaoyeli
Copy link
Owner

I was trying to update comment with my reply on github, but it doesn't seem
to work. Did you get an email with my comment update? I am repeating
them here:

On Wed, Feb 10, 2016 at 8:57 AM, Roscoe A. Bartlett <
notifications@github.com> wrote:

Sherry,

I found a few issues with the current 'master' branch Two of the issues
mentioned below are fixed by commits in the PR:

  1. The generated file make.inc is no longer being ignored. (This is fixed
    in the new PR.)

  2. The built-in CBLAS is not on by default and ignores TPL_BLAS_LIBRARIES.
    That is incompatible with the XSDK configure standard and kills library
    interoperability. When USE_XSDK_DEFAULTS=ON, it must not use the use
    the built-in CBLAS by default. (This is fixed in the new PR.)

I was testing the internal BLAS by turning ON, but forgot to change back
to OFF.

  1. There is no auotmated testing for the user example at all, even for the
    narrow use cases where it currently works. That is asking for the examples
    to be broken when users try to use it. (I did not fix this but this is not
    an issues for xSDK.)

The TESTING/ code is meant for automated testing. Once they pass, the
EXAMPLE/ code should work. The examples are convenient for users to stick
in their application codes. The EXAMPLE/Makefile needs to include
../make.inc, so the users code can easily catch the
compiler/flags/libraries-built. The problem i have now is I don't know
how to specify libsuperlu.a / libsuperlu.so, nor libblas.a / libblas.so (if
using CBLAS) in make.inc.in configure file. Can you help with that?

Can you please just accept my new pull request? This is just two simple

commits fixing the issues described above. That way I will get some credit
for contributing to SuperLU.

I just merged your 2 changes. Yesterday I was going through each of them,
just be sure I understand the issues ... Now I know how this can be done
easily.

-Ross


You can view, comment on, or merge this pull request online at:

#2
Commit Summary

  • Add ignore for generated file make.inc
  • Set enable_blaslib=OFF when USE_XSDK_DEFAULTS=ON (SDK-26)

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#2.

@bartlettroscoe
Copy link
Contributor Author

I was testing the internal BLAS by turning ON, but forgot to change back to OFF.

Note that these are CMake Cache variables, so you can just set them on the commandline like:

cmake -Denable_blaslib=ON ...

the EXAMPLE/ code should work

Famous last words :-) If there is any code or any build process that is not under automated testing, you (and users) should assume that it is broken at any point in time.

The problem i have now is I don't know how to specify libsuperlu.a / libsuperlu.so, nor libblas.a / libblas.so (if using CBLAS) in make.inc.in configure file. Can you help with that?

Yes, it is not that hard. However, further conversation on this topic should take place in a new GitHub Issue in this github repo. This PR is already closed. I will open one for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants