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

Output information after CMake configuration and generation. #610

Merged
merged 2 commits into from
Aug 8, 2020

Conversation

pratikvn
Copy link
Member

@pratikvn pratikvn commented Aug 2, 2020

This PR adds some outputs some information to the screen after CMake configuration to help the user see what configuration options have been selected. Essentially, it is a curated list of the CMakeCache.txt file.

There are two options: one being a minimal output enabled by default and the other is a detailed log which can be configured with GINKGO_CONFIG_LOG_VERBOSITY flag. Both are written to a file in the CMAKE_BINARY_DIR in any case in minimal.log and detailed.log respectively.

Additionally, the CI jobs will output the detailed log .

Closes #608 .

@pratikvn pratikvn added is:enhancement An improvement of an existing feature. reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation. 1:ST:ready-for-review This PR is ready for review is:technical-debt This is aimed at reducing technical debt. labels Aug 2, 2020
@pratikvn pratikvn self-assigned this Aug 2, 2020
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, I really like it! I mainly have a few comments and suggestions for formatting the output and structuring the output generation.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
FILE(APPEND ${minimal_log} "${ARGN}")
ENDMACRO()

FUNCTION(build_type_spec log_type var_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Do we want to deal with other build types (sanitizers)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this, I am guessing you mean printing out a log when sanitizers are being built. I see this as some quick information for the user when they configure Ginkgo. I am not sure how many actually want to run Ginkgo with sanitizers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I would only like to avoid anything breaking when we build other build types ;)

cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 2, 2020

Codecov Report

Merging #610 into develop will decrease coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #610      +/-   ##
===========================================
- Coverage    93.02%   93.01%   -0.01%     
===========================================
  Files          296      296              
  Lines        20656    20656              
===========================================
- Hits         19215    19214       -1     
- Misses        1441     1442       +1     
Impacted Files Coverage Δ
core/base/extended_float.hpp 92.23% <ø> (ø)
core/test/base/polymorphic_object.cpp 97.61% <ø> (ø)
core/test/utils/assertions.hpp 70.98% <ø> (ø)
include/ginkgo/core/base/array.hpp 92.52% <ø> (ø)
include/ginkgo/core/base/lin_op.hpp 97.64% <ø> (ø)
include/ginkgo/core/base/name_demangling.hpp 85.71% <ø> (ø)
include/ginkgo/core/base/range.hpp 95.48% <ø> (ø)
include/ginkgo/core/log/logger.hpp 92.10% <ø> (ø)
include/ginkgo/core/matrix/csr.hpp 47.69% <0.00%> (ø)
include/ginkgo/core/preconditioner/ilu.hpp 96.93% <ø> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8563fd1...cb156bc. Read the comment docs.

yhmtsai
yhmtsai previously requested changes Aug 3, 2020
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
Copy link
Member

@tcojean tcojean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small issues, in general LGTM

cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Show resolved Hide resolved
@pratikvn pratikvn added 1:ST:WIP This PR is a work in progress. Not ready for review. and removed 1:ST:ready-for-review This PR is ready for review labels Aug 5, 2020
@pratikvn pratikvn added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Aug 6, 2020
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (modulo build failures of course 😃 )

Do you think it would be possible possible to replace undefined variables by something like <unspecified>?

reference/get_info.cmake Outdated Show resolved Hide resolved
core/get_info.cmake Outdated Show resolved Hide resolved
Copy link
Member

@tcojean tcojean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The new macro based setup is really useful.

cmake/get_info.cmake Outdated Show resolved Hide resolved
@tcojean tcojean removed the 1:ST:ready-for-review This PR is ready for review label Aug 7, 2020
@tcojean tcojean added the 1:ST:ready-to-merge This PR is ready to merge. label Aug 7, 2020
@upsj
Copy link
Member

upsj commented Aug 7, 2020

About the whole "unspecified" thing, this doesn't seem to work with all variables (See HIP), maybe some stray spaces in there?

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general
need to change some nits. flags has <unspecified> but option doesn't. Is any concern on them?

cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cuda/get_info.cmake Outdated Show resolved Hide resolved
@upsj
Copy link
Member

upsj commented Aug 7, 2020

I would even suggest replacing <unspecified> by <empty>, since there seems to be no way to determine if a variable exists in CMake. or using if(DEFINED variable_name)

string(SUBSTRING
"
-- ${var_name}: " 0 55 upd_string)
if(${var_name} STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it need to be "${${var_name}}"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be correct, see Variable Expansion

hip/get_info.cmake Outdated Show resolved Hide resolved
hip/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
cmake/get_info.cmake Outdated Show resolved Hide resolved
@pratikvn
Copy link
Member Author

pratikvn commented Aug 7, 2020

About the whole "unspecified" thing, this doesn't seem to work with all variables (See HIP), maybe some stray spaces in there?

This was because of some internal variables and hence they were empty. I dont think these are necessary anyway, so i removed them.

cuda/get_info.cmake Outdated Show resolved Hide resolved
+ Update CI jobs to display the detailed logs.
Co-authored-by: Terry Cojean <[email protected]>
Co-authored-by: Tobias Ribizel <[email protected]>
Co-authored-by: Yuhsiang M. Tsai <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Aug 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (1.8.0_121) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@pratikvn pratikvn merged commit 544f7ef into develop Aug 8, 2020
@pratikvn pratikvn deleted the cmake-summary branch August 8, 2020 13:06
tcojean added a commit that referenced this pull request Aug 26, 2020
Release 1.3.0 of Ginkgo.

The Ginkgo team is proud to announce the new minor release of Ginkgo version
1.3.0. This release brings CUDA 11 support, changes the default C++ standard to
be C++14 instead of C++11, adds a new Diagonal matrix format and capacity for
diagonal extraction, significantly improves the CMake configuration output
format, adds the Ginkgo paper which got accepted into the Journal of Open Source
Software (JOSS), and fixes multiple issues.

Supported systems and requirements:
+ For all platforms, cmake 3.9+
+ Linux and MacOS
  + gcc: 5.3+, 6.3+, 7.3+, all versions after 8.1+
  + clang: 3.9+
  + Intel compiler: 2017+
  + Apple LLVM: 8.0+
  + CUDA module: CUDA 9.0+
  + HIP module: ROCm 2.8+
+ Windows
  + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, all versions after 8.1+
  + Microsoft Visual Studio: VS 2017 15.7+
  + CUDA module: CUDA 9.0+, Microsoft Visual Studio
  + OpenMP module: MinGW or Cygwin.


The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues).


Additions:
+ Add paper for Journal of Open Source Software (JOSS). [#479](#479)
+ Add a DiagonalExtractable interface. [#563](#563)
+ Add a new diagonal Matrix Format. [#580](#580)
+ Add Cuda11 support. [#603](#603)
+ Add information output after CMake configuration. [#610](#610)
+ Add a new preconditioner export example. [#595](#595)
+ Add a new cuda-memcheck CI job. [#592](#592)

Changes:
+ Use unified memory in CUDA debug builds. [#621](#621)
+ Improve `BENCHMARKING.md` with more detailed info. [#619](#619)
+ Use C++14 standard instead of C++11. [#611](#611)
+ Update the Ampere sm information and CudaArchitectureSelector. [#588](#588)

Fixes:
+ Fix documentation warnings and errors. [#624](#624)
+ Fix warnings for diagonal matrix format. [#622](#622)
+ Fix criterion factory parameters in CUDA. [#586](#586)
+ Fix the norm-type in the examples. [#612](#612)
+ Fix the WAW race in OpenMP is_sorted_by_column_index. [#617](#617)
+ Fix the example's exec_map by creating the executor only if requested. [#602](#602)
+ Fix some CMake warnings. [#614](#614)
+ Fix Windows building documentation. [#601](#601)
+ Warn when CXX and CUDA host compiler do not match. [#607](#607)
+ Fix reduce_add, prefix_sum, and doc-build. [#593](#593)
+ Fix find_library(cublas) issue on machines installing multiple cuda. [#591](#591)
+ Fix allocator in sellp read. [#589](#589)
+ Fix the CAS with HIP and NVIDIA backends. [#585](#585)

Deletions:
+ Remove unused preconditioner parameter in LowerTrs. [#587](#587)

Related PR: #625
tcojean added a commit that referenced this pull request Aug 27, 2020
The Ginkgo team is proud to announce the new minor release of Ginkgo version
1.3.0. This release brings CUDA 11 support, changes the default C++ standard to
be C++14 instead of C++11, adds a new Diagonal matrix format and capacity for
diagonal extraction, significantly improves the CMake configuration output
format, adds the Ginkgo paper which got accepted into the Journal of Open Source
Software (JOSS), and fixes multiple issues.

Supported systems and requirements:
+ For all platforms, cmake 3.9+
+ Linux and MacOS
  + gcc: 5.3+, 6.3+, 7.3+, all versions after 8.1+
  + clang: 3.9+
  + Intel compiler: 2017+
  + Apple LLVM: 8.0+
  + CUDA module: CUDA 9.0+
  + HIP module: ROCm 2.8+
+ Windows
  + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, all versions after 8.1+
  + Microsoft Visual Studio: VS 2017 15.7+
  + CUDA module: CUDA 9.0+, Microsoft Visual Studio
  + OpenMP module: MinGW or Cygwin.


The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues).


Additions:
+ Add paper for Journal of Open Source Software (JOSS). [#479](#479)
+ Add a DiagonalExtractable interface. [#563](#563)
+ Add a new diagonal Matrix Format. [#580](#580)
+ Add Cuda11 support. [#603](#603)
+ Add information output after CMake configuration. [#610](#610)
+ Add a new preconditioner export example. [#595](#595)
+ Add a new cuda-memcheck CI job. [#592](#592)

Changes:
+ Use unified memory in CUDA debug builds. [#621](#621)
+ Improve `BENCHMARKING.md` with more detailed info. [#619](#619)
+ Use C++14 standard instead of C++11. [#611](#611)
+ Update the Ampere sm information and CudaArchitectureSelector. [#588](#588)

Fixes:
+ Fix documentation warnings and errors. [#624](#624)
+ Fix warnings for diagonal matrix format. [#622](#622)
+ Fix criterion factory parameters in CUDA. [#586](#586)
+ Fix the norm-type in the examples. [#612](#612)
+ Fix the WAW race in OpenMP is_sorted_by_column_index. [#617](#617)
+ Fix the example's exec_map by creating the executor only if requested. [#602](#602)
+ Fix some CMake warnings. [#614](#614)
+ Fix Windows building documentation. [#601](#601)
+ Warn when CXX and CUDA host compiler do not match. [#607](#607)
+ Fix reduce_add, prefix_sum, and doc-build. [#593](#593)
+ Fix find_library(cublas) issue on machines installing multiple cuda. [#591](#591)
+ Fix allocator in sellp read. [#589](#589)
+ Fix the CAS with HIP and NVIDIA backends. [#585](#585)

Deletions:
+ Remove unused preconditioner parameter in LowerTrs. [#587](#587)

Related PR: #627
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. is:enhancement An improvement of an existing feature. is:technical-debt This is aimed at reducing technical debt. reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake summary after configuration
4 participants