Skip to content

cmake: set WITH_BOOST_VALGRIND before building boost#47441

Merged
cbodley merged 1 commit intoceph:mainfrom
mkogan1:wip-WITH_BOOST_VALGRIND
Aug 29, 2022
Merged

cmake: set WITH_BOOST_VALGRIND before building boost#47441
cbodley merged 1 commit intoceph:mainfrom
mkogan1:wip-WITH_BOOST_VALGRIND

Conversation

@mkogan1
Copy link
Contributor

@mkogan1 mkogan1 commented Aug 3, 2022 •
edited
Loading

https://tracker.ceph.com/issues/56500

Signed-off-by: Mark Kogan mkogan@redhat.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@mkogan1 mkogan1 requested a review from cbodley August 3, 2022 17:04
@mkogan1 mkogan1 force-pushed the wip-WITH_BOOST_VALGRIND branch from 56c48a6 to 9374eb8 Compare August 3, 2022 17:18
@mkogan1 mkogan1 changed the title build: cmake explititely log WITH_BOOST_VALGRIND config value build: cmake log WITH_BOOST_VALGRIND and WITH_SYSTEM_BOOST config values Aug 3, 2022
@mkogan1 mkogan1 requested a review from cbodley August 3, 2022 17:20
@mkogan1 mkogan1 added the DNM label Aug 3, 2022
@mkogan1
Copy link
Contributor Author

mkogan1 commented Aug 3, 2022

@cbodley investigating inconsistency in the value of WITH_BOOST_VALGRIND

in testing - even thou that changed the default of WITH_BOOST_VALGRIND to ON:
image

without any ./do_cmake.sh overrides,
the above logs show:

-- BUILDING Boost Libraries, WITH_SYSTEM_BOOST is set to OFF                                                                                                                                 
-- BUILDING Boost Libraries at j 8                                                                                                                                                           
-- BUILDING Boost Libraries, WITH_BOOST_VALGRIND is set to OFF

but ccmake ./build shows:
WITH_BOOST_VALGRIND ON

cbodley
cbodley reviewed Aug 3, 2022
View reviewed changes
@mkogan1 mkogan1 force-pushed the wip-WITH_BOOST_VALGRIND branch from 9374eb8 to 6052ada Compare August 4, 2022 09:53
@mkogan1 mkogan1 changed the title build: cmake log WITH_BOOST_VALGRIND and WITH_SYSTEM_BOOST config values build: cmake log WITH_BOOST_VALGRIND config value and fix it's out of order initialization Aug 4, 2022
@mkogan1 mkogan1 removed the DNM label Aug 4, 2022
@mkogan1 mkogan1 requested review from cbodley and yuvalif August 4, 2022 09:59
@mkogan1
Copy link
Contributor Author

mkogan1 commented Aug 4, 2022 •
edited
Loading

with config re-ordering change it possible to ad-hoc configure the value of WITH_BOOST_VALGRIND for shaman,
for example like:
sed -i 's/"Boost support for valgrind" OFF/"Boost support for valgrind" ON/' ./CMakeLists.txt ; git commit ./CMakeLists.txt -m "build: temporary WITH_BOOST_VALGRIND=ON for shaman" ; git push ci wip-rgw-40183 ; git reset --mixed HEAD~1 ; sed -i 's/"Boost support for valgrind" ON/"Boost support for valgrind" OFF/' ./CMakeLists.txt

results in:

-- Boost Libraries: WITH_SYSTEM_BOOST is set to OFF
-- BUILDING Boost Libraries at j 48
-- BUILDING Boost Libraries: WITH_BOOST_VALGRIND is set to ON

https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/64528//consoleFull

tchaikov
tchaikov requested changes Aug 4, 2022
View reviewed changes
cmake/modules/BuildBoost.cmake Outdated
message(STATUS "BUILDING Boost Libraries: WITH_BOOST_VALGRIND is set to ON")
list(APPEND b2 valgrind=on)
else()
message(STATUS "BUILDING Boost Libraries: WITH_BOOST_VALGRIND is set to OFF")
Copy link
Contributor

@tchaikov tchaikov Aug 4, 2022

Choose a reason for hiding this comment

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

I’d suggest drop this commit. These logging messages do help with debugging, but they should not get merged.

Copy link
Contributor

@yuvalif yuvalif Aug 7, 2022

Choose a reason for hiding this comment

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

but isn't that dependent on the environment (whether boost is installed or not)?
in this case it is probably good to keep the log messages.

Copy link
Contributor Author

@mkogan1 mkogan1 Aug 7, 2022

Choose a reason for hiding this comment

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

ack and ack - removed the OFF log messages to lower the log noise, the ON logs are useful for analyzing previous Shaman build logs in hindsight from the time that the boost Valgrind issues started occurring (this has happened several times)

Copy link
Contributor

@tchaikov tchaikov Aug 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

unlike the logging in python or in C++ in Ceph, where we try to understand the runtime behavior, the cmake options are explicitly passed to cmake when generating building system. so we can always understand its values by looking at the command line argument and its default value.

so, IMHO, these logging message help us to understand how CMake works, but they do not help when it comes to the behavior of our own CMake script.

Copy link
Contributor

@tchaikov tchaikov Aug 7, 2022

Choose a reason for hiding this comment

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

but isn't that dependent on the environment (whether boost is installed or not)?
in this case it is probably good to keep the log messages.

please note WITH_SYSTEM_BOOST is specified explicitly, so if it's not found. the generation phase fails. it's determined.

Copy link
Contributor

@yuvalif yuvalif Aug 7, 2022

Choose a reason for hiding this comment

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

my concern was with cases determined at compile time automatically. e.g.
https://github.com/ceph/ceph/blob/main/src/script/run-make.sh#L44

but this would be reflected in the cmake commandline, right?

Copy link
Contributor

@tchaikov tchaikov Aug 7, 2022

Choose a reason for hiding this comment

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

but this would be reflected in the cmake commandline, right?

yes, that's exactly my point.

Copy link
Contributor Author

@mkogan1 mkogan1 Aug 8, 2022

Choose a reason for hiding this comment

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

The intent of adding the log messages is operational. Problems with Valgrind have recurred several times before sporadically as a result of environmental changes and not CMake code changes, logs would assist in shortening the diagnostic cycles.

@mkogan1
Copy link
Contributor Author

mkogan1 commented Aug 4, 2022

jenkins test make check

@mkogan1 mkogan1 force-pushed the wip-WITH_BOOST_VALGRIND branch from 6052ada to abafd37 Compare August 7, 2022 10:05
@mkogan1 mkogan1 requested a review from tchaikov August 7, 2022 10:15
@yuvalif
Copy link
Contributor

yuvalif commented Aug 15, 2022

create an image that includes this PR, but also forces the build not to use system boost.
following are teuthology results:
http://pulpito.front.sepia.ceph.com/yuvalif-2022-08-15_13:31:07-rgw-wip-yuval-test-valgrind-distro-default-smithi/

no valgrind issues were detected. 4 failures are expected.

@mkogan1 mkogan1 force-pushed the wip-WITH_BOOST_VALGRIND branch from abafd37 to 358e38c Compare August 16, 2022 07:19
@mkogan1
Copy link
Contributor Author

mkogan1 commented Aug 16, 2022

@tchaikov please review, removed the log commit...

@tchaikov
Copy link
Contributor

tchaikov commented Aug 17, 2022

just one thing, the title now reads

build: cmake log WITH_BOOST_VALGRIND config value and fix it's out of order initialization

i'd suggest change it to something like:

cmake: set WITH_BOOST_VALGRIND before building boost

@mkogan1 mkogan1 force-pushed the wip-WITH_BOOST_VALGRIND branch from 358e38c to 692b99d Compare August 18, 2022 11:45
@mkogan1 mkogan1 changed the title build: cmake log WITH_BOOST_VALGRIND config value and fix it's out of order initialization cmake: set WITH_BOOST_VALGRIND before building boost Aug 18, 2022
@mkogan1
Copy link
Contributor Author

mkogan1 commented Aug 18, 2022

just one thing, the title now reads

build: cmake log WITH_BOOST_VALGRIND config value and fix it's out of order initialization

i'd suggest change it to something like:

cmake: set WITH_BOOST_VALGRIND before building boost

ack, renamed

@mkogan1
Copy link
Contributor Author

mkogan1 commented Aug 20, 2022

jenkins test api

@cbodley cbodley merged commit 183a056 into ceph:main Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tchaikov tchaikov tchaikov approved these changes

@yuvalif yuvalif yuvalif approved these changes

@cbodley cbodley Awaiting requested review from cbodley

Assignees

No one assigned

Labels

build/ops

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mkogan1 @yuvalif @tchaikov @cbodley