Properly find Valgrind on OpenSUSE by lukaszstolarczuk · Pull Request #961 · pmem/libpmemobj-cpp

lukaszstolarczuk · 2020-10-30T10:35:57Z

This change is

lukaszstolarczuk

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion

utils/docker/images/Dockerfile.opensuse-leap-latest, line 74 at r1 (raw file):

COPY install-valgrind.sh install-valgrind.sh
RUN ./install-valgrind.sh
ENV PKG_CONFIG_PATH $PKG_CONFIG_PATH:/usr/lib/pkgconfig/

perhaps instead of that, we should update CMake somehow to look in path not only /usr/lib64/ but also in /usr/lib/ (which is the case here, I guess)

codecov · 2020-10-30T10:58:57Z

Codecov Report

Merging #961 (441d42b) into master (4891cbd) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
- Coverage   95.87%   95.79%   -0.08%     
==========================================
  Files          48       48              
  Lines        6258     6258              
==========================================
- Hits         6000     5995       -5     
- Misses        258      263       +5

Flag	Coverage Δ
tests_clang_debug_cpp17	`95.42% <ø> (-0.04%)`	⬇️
tests_gcc_debug	`92.42% <ø> (+0.01%)`	⬆️

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%> (-4.77%)`	⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp	`93.03% <0.00%> (-0.20%)`	⬇️
...j++/container/detail/concurrent_skip_list_impl.hpp	`97.18% <0.00%> (-0.14%)`	⬇️

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 4891cbd...441d42b. Read the comment docs.

igchor

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion

karczex · 2020-11-03T12:10:49Z

utils/docker/images/Dockerfile.opensuse-leap-latest, line 74 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

perhaps instead of that, we should update CMake somehow to look in path not only /usr/lib64/ but also in /usr/lib/ (which is the case here, I guess)

+1

karczex

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)

lukaszstolarczuk · 2020-11-06T16:39:46Z

Updated as discussed, if 'TESTS_USE_VALGRIND' is ON and no Valgrind found in the system - fail the build.
Also, I've updated Valgrind checks on Windows.

szyrom

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

lukaszstolarczuk

Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk and @szyrom)

utils/docker/run-build.sh, line 347 at r3 (raw file):

	cmake .. -DCMAKE_BUILD_TYPE=Release \
		-DCMAKE_INSTALL_PREFIX=$INSTALL_DIR \
		-DTESTS_USE_VALGRIND=OFF \

actually - is it alright that there's no valgrind found in this build? I had to add this line, because it threw me "my new cmake exception" - "valgrind not found, but TESTS_USE_VAL was set to ON"...? is this expected? I'm not sure...

karczex · 2020-11-20T09:08:22Z

utils/docker/images/Dockerfile.opensuse-tumbleweed-latest, line 76 at r3 (raw file):

COPY install-valgrind.sh install-valgrind.sh
RUN ./install-valgrind.sh
ENV PKG_CONFIG_PATH $PKG_CONFIG_PATH:/usr/lib/pkgconfig/

You probably don't need to include /usr/lib/pkgconfig. According to pkg-config man page:

The default directory will always be searched after searching the path;

lukaszstolarczuk

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @karczex, @lukaszstolarczuk, and @szyrom)

utils/docker/images/Dockerfile.opensuse-tumbleweed-latest, line 76 at r3 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

You probably don't need to include /usr/lib/pkgconfig. According to pkg-config man page:
The default directory will always be searched after searching the path; 

/usr/lib/pkgconfig/ is the path that was used by Valgrind installation and it then couldn't be find in our run-build.sh. So we need to add exactly this path - the default one on this OS is lib64, I believe. Also, see my comment above

igchor

Reviewed 1 of 4 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karczex and @lukaszstolarczuk)

utils/docker/run-build.sh, line 224 at r3 (raw file):

	mkdir build && cd build

	echo "---------------------------- Error expected! ------------------------------"

Shouldn't this test be created as a separate functions (instead of inside tests_gcc_release_cpp17_no_valgrind)? Not sure if n jenkins, we might want to run something which messses with the system files?

utils/docker/run-build.sh, line 347 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

actually - is it allright that there's no valgrind found in this buikd? I had to add this line, because it threw me "my new cmake exception" - "valgrind not found, but TESTS_USE_VAL was set to ON"...? is this expected? I'm not sure...

I guess it's OK, since valgrind is only found through pkg-config

utils/docker/images/Dockerfile.opensuse-leap-latest, line 74 at r1 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

+1

But we're relying on pkg-config, not cmake to find the package.

lukaszstolarczuk

Reviewable status: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @igchor, @karczex, @lukaszstolarczuk, and @szyrom)

utils/docker/run-build.sh, line 224 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Shouldn't this test be created as a separate functions (instead of inside tests_gcc_release_cpp17_no_valgrind)? Not sure if n jenkins, we might want to run something which messses with the system files?

Done.

utils/docker/images/Dockerfile.opensuse-leap-latest, line 74 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

But we're relying on pkg-config, not cmake to find the package.

I think we should make it possible, right now we try to find it like this (in top-level CMake):

# Find Valgrind
if(PKG_CONFIG_FOUND)
	pkg_check_modules(VALGRIND QUIET valgrind)
else()
	find_package(VALGRIND QUIET)
endif()

so I believe, I'll file an issue to try and deliver FindValgrind script, because it seems not to work without pkg-config. It could be fixed with something like this:
https://github.com/elemental/Elemental/blob/master/cmake/modules/FindValgrind.cmake

And update check in run-build.sh to test if build really fails.

and make sure to skip only Valgrind tests, not for all tracers.

igchor

Reviewable status: 1 of 7 files reviewed, 3 unresolved discussions (waiting on @igchor, @karczex, @lukaszstolarczuk, and @szyrom)

utils/docker/images/Dockerfile.opensuse-leap-latest, line 74 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I think we should make it possible, right now we try to find it like this (in top-level CMake):
# Find Valgrind
if(PKG_CONFIG_FOUND)
	pkg_check_modules(VALGRIND QUIET valgrind)
else()
	find_package(VALGRIND QUIET)
endif()
so I believe, I'll file an issue to try and deliver FindValgrind script, because it seems not to work without pkg-config. It could be fixed with something like this:
https://github.com/elemental/Elemental/blob/master/cmake/modules/FindValgrind.cmake

But the main reason for using cmake to find packages is windows, on all other OSes we use pkg-config. Since valgrind is not avaialble on windows I don't see a reason for that.

lukaszstolarczuk commented Oct 30, 2020

View reviewed changes

igchor approved these changes Nov 2, 2020

View reviewed changes

karczex reviewed Nov 3, 2020

View reviewed changes

lukaszstolarczuk marked this pull request as draft November 5, 2020 13:32

lukaszstolarczuk marked this pull request as ready for review November 6, 2020 16:01

lukaszstolarczuk force-pushed the fix-opensuse-valgrind-installation branch 2 times, most recently from 06082a8 to 61d7ec5 Compare November 6, 2020 16:34

szyrom reviewed Nov 18, 2020

View reviewed changes

lukaszstolarczuk force-pushed the fix-opensuse-valgrind-installation branch from 61d7ec5 to 3576ed9 Compare November 18, 2020 16:41

lukaszstolarczuk commented Nov 19, 2020

View reviewed changes

lukaszstolarczuk commented Nov 20, 2020

View reviewed changes

igchor reviewed Dec 8, 2020

View reviewed changes

lukaszstolarczuk force-pushed the fix-opensuse-valgrind-installation branch from 3576ed9 to de4779d Compare December 10, 2020 13:57

lukaszstolarczuk commented Dec 10, 2020

View reviewed changes

lukaszstolarczuk mentioned this pull request Dec 10, 2020

Find Valgrind without pkg-config #992

Closed

lukaszstolarczuk added 4 commits December 10, 2020 17:24

docker: set pkg config path to find Valgrind in OpenSUSE images

f480628

docker: use sys env PKG_CONFIG_PATH in builds

c7ca699

if Valgrind tests suppose to run, fail build if Valgrind not found

5e0ec77

And update check in run-build.sh to test if build really fails.

cmake: add warning when Valgrind tests are enabled and on Windows

441d42b

and make sure to skip only Valgrind tests, not for all tracers.

lukaszstolarczuk force-pushed the fix-opensuse-valgrind-installation branch from de4779d to 441d42b Compare December 10, 2020 16:40

igchor reviewed Dec 16, 2020

View reviewed changes

igchor merged commit f4dbff8 into pmem:master Dec 16, 2020

lukaszstolarczuk deleted the fix-opensuse-valgrind-installation branch December 16, 2020 10:57

Conversation

lukaszstolarczuk commented Oct 30, 2020 • edited by marcinslusarz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

karczex commented Nov 3, 2020

Uh oh!

karczex left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk commented Nov 6, 2020

Uh oh!

szyrom left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karczex commented Nov 20, 2020

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukaszstolarczuk commented Oct 30, 2020 •

edited by marcinslusarz

Loading

codecov Bot commented Oct 30, 2020 •

edited

Loading

lukaszstolarczuk left a comment •

edited

Loading