Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

Update cmake_build_type handling and switch default to RelWithDebInfo#1044

Merged
igchor merged 6 commits intopmem:masterfrom
lukaszstolarczuk:change-default-build-type
Mar 1, 2021
Merged

Update cmake_build_type handling and switch default to RelWithDebInfo#1044
igchor merged 6 commits intopmem:masterfrom
lukaszstolarczuk:change-default-build-type

Conversation

@lukaszstolarczuk
Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk commented Feb 22, 2021

This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 22, 2021

Codecov Report

Merging #1044 (003a6e0) into master (a10ad3b) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
+ Coverage   93.99%   94.07%   +0.08%     
==========================================
  Files          48       48              
  Lines        4711     4746      +35     
==========================================
+ Hits         4428     4465      +37     
+ Misses        283      281       -2     
Flag Coverage Δ
tests_clang_debug_cpp17 93.41% <ø> (+0.22%) ⬆️
tests_gcc_debug 92.05% <ø> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/libpmemobj++/condition_variable.hpp 73.80% <0.00%> (-2.39%) ⬇️
include/libpmemobj++/string_view.hpp 100.00% <0.00%> (ø)
...ude/libpmemobj++/container/concurrent_hash_map.hpp 93.88% <0.00%> (+0.15%) ⬆️
include/libpmemobj++/transaction.hpp 82.56% <0.00%> (+0.91%) ⬆️
...libpmemobj++/detail/enumerable_thread_specific.hpp 100.00% <0.00%> (+1.08%) ⬆️

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 a10ad3b...003a6e0. Read the comment docs.

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, all discussions resolved

@lukaszstolarczuk lukaszstolarczuk marked this pull request as draft February 22, 2021 14:59
@lukaszstolarczuk lukaszstolarczuk force-pushed the change-default-build-type branch 5 times, most recently from bda8a6b to 3c07582 Compare February 23, 2021 14:45
@lukaszstolarczuk lukaszstolarczuk marked this pull request as ready for review February 23, 2021 15:23
@lukaszstolarczuk lukaszstolarczuk force-pushed the change-default-build-type branch from 3c07582 to ecc560a Compare February 24, 2021 09:14
Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)


CMakeLists.txt, line 57 at r2 (raw file):

	message(STATUS "CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}")
	if(NOT CMAKE_BUILD_TYPE IN_LIST predefined_build_types)
		message(WARNING "Unusual build type was set, please make sure it's proper one. "

Unusual is MinSizeRel, but something from out of the list is definitely "Wrong build type"

Copy link
Copy Markdown
Member Author

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KFilipek)


CMakeLists.txt, line 57 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Unusual is MinSizeRel, but something from out of the list is definitely "Wrong build type"

I'd rather disagree. I understand your point, but:

  • CMake may extend the list of supported build types, and I'd rather be angry if CMake (of any project) forced fail me ;)
  • anyone can extend this list and define their own build type with their own compiler flags

This warning is rather for people who mistyped some name, or use uppercase where there isn't... something like this.

Copy link
Copy Markdown

@karczex karczex left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, 1 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KFilipek)

@lukaszstolarczuk lukaszstolarczuk marked this pull request as draft February 25, 2021 09:46
in the past some users run e.g. benchmarks and was surprised with
the results. Using release version should omit that problem.
Documentation is updated as well to point users with proper CMake parameter.
- make use of STRINGS keyword to display better hint for the CMake param (in cmake gui),
- warn if unusual build type chosen.
@lukaszstolarczuk lukaszstolarczuk force-pushed the change-default-build-type branch from ecc560a to 003a6e0 Compare February 25, 2021 11:26
@lukaszstolarczuk lukaszstolarczuk marked this pull request as ready for review February 25, 2021 12:40
Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)


CMakeLists.txt, line 57 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'd rather disagree. I understand your point, but:

  • CMake make extend the list of supported build types, and I'd rather be angry if CMake (of any project) forced fail me ;)
  • anyone can extend this list and define their own build type with their own compiler flags

This warning is rather for people who mistyped some name, or use uppercase where there isn't... something like this.

Explained in offline discussion

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)

@igchor igchor merged commit 515f097 into pmem:master Mar 1, 2021
@lukaszstolarczuk lukaszstolarczuk deleted the change-default-build-type branch March 1, 2021 08:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants