-
Notifications
You must be signed in to change notification settings - Fork 30
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
Fixed lcov error #771
Fixed lcov error #771
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change, but otherwise looks good to me. Please make sure to run CI on both Noble and Jammy to double check this works for both.
Could this solve: #770? |
@Crola1702 yes, It's a fix for this issue |
@clalancette I think i fixed this build, coverage is here https://ci.ros2.org/view/nightly/job/nightly_linux_coverage/2361/cobertura/. Do you mind to take another look to the changes? |
So I'm not totally sure about this, but this may not be producing correct results. In particular, if you look at: https://ci.ros2.org/view/nightly/job/nightly_linux_iron_coverage/408/cobertura/src_ros2_rcutils_src/allocator_c/allocator_c/ , you'll see that everything looks like we expect. But if you look at https://ci.ros2.org/view/nightly/job/nightly_linux_coverage/2361/cobertura/src_ros2_rcutils_src/allocator_c/allocator_c/ (from the CI here), you'll see some functions with numbers in front of them, like I have no idea if that is what |
The problem with |
Yeah, understood. We can at least pull the code from https://pypi.org/project/lcov-cobertura-fix/#files , and compare it to a "pristine" lcov_corbertua to see what the differences are. |
Coverages are more simular right now
But I can see this error now:
any thoughts? |
related PR eriwen/lcov-to-cobertura-xml#55 |
|
@clalancette assigning you as it looks like @ahcorde is awaiting your thoughts 🧇 |
linux_docker_resources/Dockerfile
Outdated
@@ -144,7 +145,7 @@ RUN if test ${COMPILE_WITH_CLANG} = true; then apt-get update && apt-get install | |||
# Install coverage build dependencies. | |||
RUN apt-get update && apt-get install --no-install-recommends -y lcov | |||
|
|||
RUN pip3 install -U lcov_cobertura_fix | |||
RUN pip3 install -U lcov_cobertura && rm /usr/local/lib/python3.12/dist-packages/lcov_cobertura/lcov_cobertura.py && wget https://raw.githubusercontent.com/ahcorde/lcov-to-cobertura-xml/master/lcov_cobertura/lcov_cobertura.py -P /usr/local/lib/python3.12/dist-packages/lcov_cobertura/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move forward with a fork of this fix for now, but we should do that from an repository. We should also add in a comment here explaining exactly why we are doing that.
Let me discuss today with some people to decide where we should put this fork for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I talked to @nuclearsandwich about this, and I got a much better idea of what we should do here. I think the easiest thing is to just use your fork for now, since you already have a fork. In that case, I think this should be:
RUN pip3 install -U lcov_cobertura && rm /usr/local/lib/python3.12/dist-packages/lcov_cobertura/lcov_cobertura.py && wget https://raw.githubusercontent.com/ahcorde/lcov-to-cobertura-xml/master/lcov_cobertura/lcov_cobertura.py -P /usr/local/lib/python3.12/dist-packages/lcov_cobertura/ | |
# There is a bug in upstream lcov-to-cobertura-xml with newer versions of lcov. There is an | |
# open PR for it in https://github.com/eriwen/lcov-to-cobertura-xml/pull/55, but until that is | |
# merged use the fork with the fix. | |
RUN pip3 install -U git+https://github.com/ahcorde/lcov-to-cobertura-xml@master |
lcov_arguments = ['--ignore-errors', 'inconsistent,inconsistent', | ||
'--ignore-errors', 'mismatch,mismatch', | ||
'--ignore-errors', 'negative,negative', | ||
'--ignore-errors', 'unused,unused', | ||
'--ignore-errors', 'empty,empty'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against this, but can you explain more why we need to ignore all of these errors? I think it would be best to have a comment here with that, so future readers know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just launching the CI and fixing the errors.
linux_docker_resources/Dockerfile
Outdated
@@ -144,7 +145,7 @@ RUN if test ${COMPILE_WITH_CLANG} = true; then apt-get update && apt-get install | |||
# Install coverage build dependencies. | |||
RUN apt-get update && apt-get install --no-install-recommends -y lcov | |||
|
|||
RUN pip3 install -U lcov_cobertura_fix | |||
RUN pip3 install -U lcov_cobertura && rm /usr/local/lib/python3.12/dist-packages/lcov_cobertura/lcov_cobertura.py && wget https://raw.githubusercontent.com/ahcorde/lcov-to-cobertura-xml/master/lcov_cobertura/lcov_cobertura.py -P /usr/local/lib/python3.12/dist-packages/lcov_cobertura/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I talked to @nuclearsandwich about this, and I got a much better idea of what we should do here. I think the easiest thing is to just use your fork for now, since you already have a fork. In that case, I think this should be:
RUN pip3 install -U lcov_cobertura && rm /usr/local/lib/python3.12/dist-packages/lcov_cobertura/lcov_cobertura.py && wget https://raw.githubusercontent.com/ahcorde/lcov-to-cobertura-xml/master/lcov_cobertura/lcov_cobertura.py -P /usr/local/lib/python3.12/dist-packages/lcov_cobertura/ | |
# There is a bug in upstream lcov-to-cobertura-xml with newer versions of lcov. There is an | |
# open PR for it in https://github.com/eriwen/lcov-to-cobertura-xml/pull/55, but until that is | |
# merged use the fork with the fix. | |
RUN pip3 install -U git+https://github.com/ahcorde/lcov-to-cobertura-xml@master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more change. Once that is in, I'm going to suggest we run the coverage jobs for all of Humble, Iron, Jazzy, and Rolling to ensure that things are working across all of them.
linux_docker_resources/Dockerfile
Outdated
yamllint \ | ||
wget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this change anymore, since we aren't using wget
anymore.
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
59ebc69
to
302b4ff
Compare
All right, this now looks good to me. Since the PR still hasn't merged upstream, let's go with Alex's fork for now, and then when it is eventually merged and released upstream we can use that. |
There is an error on CI https://ci.ros2.org/view/nightly/job/nightly_linux_coverage/2348/consoleText related with
lcov
, this should fix the problem