[pettershao-ragilenetworks]

Signed-off-by: pettershao-ragilenetworks pettershao@ragilenetworks.com

[kcudnik]

tests probably should not have gcov enabled on them ?

[pettershao-ragilenetworks]

yea, will optimize them soon.

[pettershao-ragilenetworks]

@kcudnik https://dev.azure.com/mssonic/build/_build/results?buildId=54671&view=codecoverage-tab the result seems fine, 80%, please check if we need to remove some folders, thanks!

[kcudnik]

@kcudnik https://dev.azure.com/mssonic/build/_build/results?buildId=54671&view=codecoverage-tab the result seems fine, 80%, please check if we need to remove some folders, thanks!

wow, this looks impressive, and we are over the 80% target ! this is great !
4 main directories that we want to cover are

• lib
• meta
• syncd
• vslib

and i see that this is already covered, and i see that you also covered tests/ and unittests/ which are also fine ! and there are alscovered saiasiccmp/ and saiplayer/ which is also good

thank you @pettershao-ragilenetworks, this is really great work, and we can use this method to report coverage over local unittests which cover currently 60%, and using this will meet MS requirement and we will be done for a while :D

[pettershao-ragilenetworks]

@kcudnik it already include unittest coverage rate, I have merged unittest coverage rate and vs docker coverage together(by generate .info for unittest and .info for vs docker, and merge them together), you can see that 'test' and 'unittest' folders are come from unittest（i just fix the path prefix for merging and showing）, and other folders are come from vs docker.

[pettershao-ragilenetworks]

@kcudnik help forward this, thanks!

[kcudnik]

what do you need from me here?

[pettershao-ragilenetworks]

what do you need from me here?

I think we can merge this, right? happy new year!

[pettershao-ragilenetworks]

@lguohan @qiluo-msft hi, help merge this, thanks!

[kcudnik]

lgtm is failing and 2 pipelines for arm64 and armhf are stuck, not sure if they are required

[pettershao-ragilenetworks]

@theasianpianist @prsunny can you help check why vstest fail so much?

[talber-nvidia]

Hi, we were wondering- when is this PR expected to be merged?

[liat-grozovik]

@pettershao-ragilenetworks would you please help to handle conflicts and retrigger checkers?

[pettershao-ragilenetworks]

OK. will handle soon.

[pettershao-ragilenetworks]

lgtm exit abnormal, any one please help check this:
[2022-05-18 09:52:02] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/opt/dist/tools/linux64/preload_tracer, /opt/dist/cpp/tools/do-build])
[2022-05-18 09:52:02] [build-stdout] Semmle autobuild: no supported build system detected.

[pettershao-ragilenetworks]

@kcudnik @lguohan help check why lgtm failed, i don't think it is caused by my submission, thanks!

[kcudnik]

[2022-05-18 09:52:02] [build-stdout] sudo cp libgcovpreload.so /usr/lib
[2022-05-18 09:52:02] [build-stderr] sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper

seems like sudo don't work on LGTM, and it requires password, probably is not supported, at all, so that copy should not be used there

[pettershao-ragilenetworks]

libgcovpreload

any idea to fix this? It seems fine last time, can we just skip lgtm since we have not change any code.
cat gcovpreload/Makefile

DYLIB_MAKE_CMD=$(CC) -shared -fpic gcovpreload.c -o ${DYLIBNAME}

all:
        $(DYLIB_MAKE_CMD)
        sudo cp $(DYLIBNAME) /usr/lib     -> hear
        sudo chmod 777 -R /usr/lib/$(DYLIBNAME)   -> hear
        sudo cp lcov_cobertura.py ../    -> hear

[kcudnik]

if we skip this time, error will still persist on changes where code is changed, so it make sense to fix it here and not in next PR

[pettershao-ragilenetworks]

yea, i understand, but if not skip this, then seems no other solution:

1. all other components' make depend this so, so the so's make is like make install, this need 'sudo'
2. if we make and install this so alone, then we should create a folder for gcovpreload.so's building, that means we need to modify build-image, seems complex.
3. seems no changes will make for this Makefile after this PR
   help check.

[kcudnik]

hmm, seems like there is no good way for LGTM ice sudo will not work, besides maybe to skip installing that so just for LGTM build, since that only check code quality and not running anything else.
maybe it's worth to make a small Makefile just for LGTM to check whether make install will work or explicit sudo, but i suspect this will not work :(
other workaround, maybe would be to specify explicit lib folder for install so libraries and use that so folder in ld_preload, just for LGTM

hmm, that sudo should work in LGTM, since it's in container, not sure why it started to require password :/

[kcudnik]

if you look here:
https://github.com/Azure/sonic-sairedis/blob/master/lgtm.yml#L25
or https://github.com/Azure/sonic-sairedis/blob/master/lgtm.yml#L30
this points that everything is installed in LGTM_WORKSPACE directory, and not using system directories, and that resolves to /opt/work/lgtm-workspace, so that "sudo" maybe should use "make install" command which would automatically put required library in the prefix directory, currently for sairedis that prefix is set to /usr, but sairedis is not installed, just configure

you would need to modify gcovpreload/Makefile, "all" to use prefix, and change change prefix from /usr in lgtm.yml, to something that includes LGTM_WORKSPACE, since you have hardcoded

for example if you look at generated Makefile:

exec_prefix = ${prefix}
bindir = ${exec_prefix}/bin
libdir = ${exec_prefix}/lib

and prefix is obtained from --prefix in ./configure, you could also use that, ps why you create your own Makefile and mock all targets, instead using Makefile generated by ./configure which would take care of everything for you? you just need to create very simple Makefile.am that will have one source file for preload lib and that's it ?

[pettershao-ragilenetworks]

OK, I will raise a PR to create gcovpreloader repo soon and add build dependance for sairedis(swss will also depend it), after the pr raised, I will summarize reasons and send a mail for forwarding this.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: pettershao-ragilenetworks (c5d0390, 1eb71e0, c74e71a, 64234e3, e5a6cf8, a278cf1, a556021, c3da170, d97e588, 301e885, 91adb69, 348bf31, 37326be, 959ec32, c970c2a, 43d0ced, a979f3b, 8630dad, e4aa8fe, 56e6009, cbff883, 55887ab, e76705c, 5a3e0d7, 50ed0ea, a74abff, aeba308, 7effce8, a616fe2, ed69f67, e62d713, e52a1e1, 88b55d3, fa30441, ebe72b1, 7dd927b, 09ff654, 355947e, d4e7be2)
• ✅ login: sumanbrcm / name: Suman Kumar (3bb40a9)
• ✅ login: richardyu-ms / name: Richard.Yu (95891d7)
• ✅ login: theasianpianist / name: Lawrence Lee (707c2a0)

[pettershao-ragilenetworks]

Should this PR be merged? @kcudnik @liat-grozovik

[Wick324]

/easycla

[pettershao-ragilenetworks]

/easycla

[pettershao-ragilenetworks]

/easycla

[pettershao-ragilenetworks]

if you look here: https://github.com/Azure/sonic-sairedis/blob/master/lgtm.yml#L25 or https://github.com/Azure/sonic-sairedis/blob/master/lgtm.yml#L30 this points that everything is installed in LGTM_WORKSPACE directory, and not using system directories, and that resolves to /opt/work/lgtm-workspace, so that "sudo" maybe should use "make install" command which would automatically put required library in the prefix directory, currently for sairedis that prefix is set to /usr, but sairedis is not installed, just configure

you would need to modify gcovpreload/Makefile, "all" to use prefix, and change change prefix from /usr in lgtm.yml, to something that includes LGTM_WORKSPACE, since you have hardcoded

for example if you look at generated Makefile:

exec_prefix = ${prefix}
bindir = ${exec_prefix}/bin
libdir = ${exec_prefix}/lib

and prefix is obtained from --prefix in ./configure, you could also use that, ps why you create your own Makefile and mock all targets, instead using Makefile generated by ./configure which would take care of everything for you? you just need to create very simple Makefile.am that will have one source file for preload lib and that's it ?

I have create a Makefile.am to replace the Makefile , it works fine and passed the lgtm check.
Here is the result: https://dev.azure.com/mssonic/build/_build/results?buildId=182795&view=codecoverage-tab
Please kindly review the changes and then we could merge if there is no problems.

[lgtm-com]

This pull request introduces 1 alert when merging 9199a36 into 4e24c77 - view on LGTM.com

new alerts:

• 1 for Function declared in block

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.