fix: rust coverage test meets compile error for missing debuginfo #1740

yihau · 2023-04-20T05:19:24Z

Hi @Xuanwo, does this one look correct?

codecov-commenter · 2023-04-20T05:33:12Z

Codecov Report

Patch coverage: 47.05% and project coverage change: +0.25 🎉

Comparison is base (4a8b5f5) 28.87% compared to head (9bbf8b4) 29.12%.

❗ Current head 9bbf8b4 differs from pull request most recent head 10b37a7. Consider uploading reports for the commit 10b37a7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1740      +/-   ##
==========================================
+ Coverage   28.87%   29.12%   +0.25%     
==========================================
  Files          49       49              
  Lines       17202    17468     +266     
  Branches     8268     8470     +202     
==========================================
+ Hits         4967     5088     +121     
- Misses       7221     7297      +76     
- Partials     5014     5083      +69

Impacted Files	Coverage Δ
src/compiler/rust.rs	`33.37% <47.05%> (+0.51%)`	⬆️

... and 21 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

sylvestre · 2023-04-20T05:50:38Z

I would like to see a test for that please. Thanks

Xuanwo · 2023-04-20T06:25:36Z

I don't know how optional works, maybe it ok for us to ignore all errors happened while writing cache?

yihau · 2023-04-20T08:47:38Z

I found the feature was introduced cuz .dwo files may absent.

sccache/src/compiler/gcc.rs

Lines 536 to 542 in 4a8b5f5

    
           outputs.insert( 
        
               "dwo", 
        
               ArtifactDescriptor { 
        
                   path: dwo, 
        
                   optional: true, 
        
               }, 
        
           );

in the same PR, the author try to keep .gcno having the original behavior I think. so he make:

sccache/src/compiler/gcc.rs

Lines 550 to 556 in 4a8b5f5

    
           outputs.insert( 
        
               "gcno", 
        
               ArtifactDescriptor { 
        
                   path: gcno, 
        
                   optional: false, 
        
               }, 
        
           );

tbh I have no idea how sccache deal with those .gcno files. maybe .gcno are also optional files?

yihau · 2023-04-20T09:05:35Z

yeah. I think .gcno may absent as well in the newer nightly rust version.

I found that when we try to do coverage test,

in nightly-2023-02-08, all file in deps have .d, .gcda, .gcno and -cgu.0.rcgu.o. 4 different extensions file.

but in nightly-2023-02-09, some of deps only have .d file

btw, I tested it in this repo https://github.com/yihau/sccache-coverage-build-error

This reverts commit 30b616e.

yihau

copied the mechanism from c complier

src/compiler/rust.rs

Xuanwo · 2023-04-20T10:03:29Z

Mostly LGTM! Can you add a job which uses rust coverage to make sure it works? I think we can add a new job called issues-1652.

Xuanwo

Mostly LGTM, thanks a lot!

ryoqun · 2023-04-20T13:04:35Z

hi, thanks for quick review! I'm from the same org (@solana-labs) with @yihau

X ci / test freebsd-13.1 rust stable (macos-12, 13.1) (pull_request) Failing after 18m

i couldn't decipher the error from the ci build...

in nightly-2023-02-08, all file in deps have .d, .gcda, .gcno and -cgu.0.rcgu.o. 4 different extensions file.

but in nightly-2023-02-09, some of deps only have .d file

so, it looks like the regression window is quite narrow: rust-lang/rust@bd39bbb...ef934d9

among them, this came to my attention: rust-lang/rust@6f38fd5, rust-lang/rust#107778

which in turn contains this: rust-lang/cargo#11252

Xuanwo · 2023-04-20T13:08:47Z

i couldn't decipher the error from the ci build...

I believe this CI failure is not related to this PR. Please ignore it.

sylvestre · 2023-04-21T05:48:36Z

.github/workflows/integration-tests.yml

@@ -609,3 +609,32 @@ jobs:
          ${SCCACHE_PATH} --show-stats

          ${SCCACHE_PATH} --show-stats | grep -e "Cache hits\s*[1-9]"
+
+  issues-1652:


Could you please rename the test for something more explicit?

do you have any prefer name? is missing-debug-info a good name?

sylvestre · 2023-04-21T05:49:03Z

.github/workflows/integration-tests.yml

+    strategy:
+      fail-fast: false
+      matrix:
+        version: [nightly-2023-02-08, nightly-2023-02-09]


Is it necessary to hard-coded the date?

I found it's broken since the version nightly-2023-02-09 so I choose those two version to test.

sylvestre · 2023-04-21T05:56:54Z

I would prefer that you extend it to test coverage in general Le ven. 21 avr. 2023, 07:53, Yihau Chen ***@***.***> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In .github/workflows/integration-tests.yml <#1740 (comment)>: > @@ -609,3 +609,32 @@ jobs: ${SCCACHE_PATH} --show-stats ${SCCACHE_PATH} --show-stats | grep -e "Cache hits\s*[1-9]" + + issues-1652: do you have any prefer name? is missing-debug-info a good name? — Reply to this email directly, view it on GitHub <#1740 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFTBDSMVDR5X6RWXYTWSXDXCIOG5ANCNFSM6AAAAAAXE7DNKE> . You are receiving this because you commented.Message ID: ***@***.***>

yihau · 2023-04-21T06:02:50Z

oh. do you mean that 1) change job name to test coverage and 2) use the latest nightly version ?

yihau · 2023-04-21T09:03:51Z

Hi @sylvestre, I have a diff in my local. it looks like:

diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml
index cf3e909..3ad4cbe 100644
--- a/.github/workflows/integration-tests.yml
+++ b/.github/workflows/integration-tests.yml
@@ -610,14 +610,9 @@ jobs:

           ${SCCACHE_PATH} --show-stats | grep -e "Cache hits\s*[1-9]"

-  issues-1652:
+  rust-test-coverage:
     runs-on: ubuntu-latest
     needs: build
-
-    strategy:
-      fail-fast: false
-      matrix:
-        version: [nightly-2023-02-08, nightly-2023-02-09]
     steps:
       - uses: actions/download-artifact@v3
         with:
@@ -628,11 +623,11 @@ jobs:

       - name: Coverage test
         run: |
-          rustup toolchain install ${{ matrix.version }}
+          rustup toolchain install nightly
           cargo new hello
           cd hello
           echo "serde = { version = \"1.0\", features = [\"derive\"] }" >> Cargo.toml
-          cargo +${{ matrix.version }} test
+          cargo +nightly test
         env:
           RUSTC_WRAPPER: /home/runner/.cargo/bin/sccache
           CARGO_INCREMENTAL: "0"

I will push this code if you think it is better

sylvestre · 2023-04-21T09:06:16Z

yeah, exactly what I was expecting :)

.github/workflows/integration-tests.yml

Xuanwo · 2023-04-24T04:39:24Z

This PR seems ready to go now. @sylvestre Do you wanna to take another look?

ryoqun · 2023-04-25T13:20:41Z

hey, it looks like v0.4.2 has shipped yesterday and includes this pr. really thanks for it. but i'm wondering why v0.4.2 isn't still appearing at crates.io? (we use that release channel for convenience as part of our docker ci image generation).

Xuanwo · 2023-04-25T13:22:26Z

seems we didn't push to crates.io in our release workflow.

ignore file not found error

30b616e

yihau added 2 commits April 20, 2023 17:56

Revert "ignore file not found error"

b9fb161

This reverts commit 30b616e.

make gcno as optional files in rust compiler

42eac8c

yihau commented Apr 20, 2023

View reviewed changes

src/compiler/rust.rs Show resolved Hide resolved

src/compiler/rust.rs Outdated Show resolved Hide resolved

yihau added 2 commits April 20, 2023 18:08

fix redundant clone

639976b

add integration test, issues-1652

d505f65

Xuanwo approved these changes Apr 20, 2023

View reviewed changes

Xuanwo changed the title ~~ignore file not found error~~ fix: rust coverage test meets compile error for missing debuginfo Apr 20, 2023

sylvestre reviewed Apr 21, 2023

View reviewed changes

make test more general

adcec3a

sylvestre reviewed Apr 21, 2023

View reviewed changes

.github/workflows/integration-tests.yml Outdated Show resolved Hide resolved

yihau added 3 commits April 21, 2023 17:57

move env to job level

3e0a326

test coverage twice and print status

9bbf8b4

fix working directory

10b37a7

sylvestre merged commit 3b70d6b into mozilla:main Apr 24, 2023
37 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rust coverage test meets compile error for missing debuginfo #1740

fix: rust coverage test meets compile error for missing debuginfo #1740

yihau commented Apr 20, 2023 •

edited

codecov-commenter commented Apr 20, 2023 •

edited

sylvestre commented Apr 20, 2023

Xuanwo commented Apr 20, 2023

yihau commented Apr 20, 2023 •

edited

yihau commented Apr 20, 2023 •

edited

yihau left a comment

Xuanwo commented Apr 20, 2023

Xuanwo left a comment

ryoqun commented Apr 20, 2023

Xuanwo commented Apr 20, 2023

sylvestre Apr 21, 2023

yihau Apr 21, 2023 •

edited

sylvestre Apr 21, 2023

yihau Apr 21, 2023

sylvestre commented Apr 21, 2023 via email

yihau commented Apr 21, 2023

yihau commented Apr 21, 2023

sylvestre commented Apr 21, 2023

Xuanwo commented Apr 24, 2023

ryoqun commented Apr 25, 2023

Xuanwo commented Apr 25, 2023

fix: rust coverage test meets compile error for missing debuginfo #1740

fix: rust coverage test meets compile error for missing debuginfo #1740

Conversation

yihau commented Apr 20, 2023 • edited

codecov-commenter commented Apr 20, 2023 • edited

Codecov Report

sylvestre commented Apr 20, 2023

Xuanwo commented Apr 20, 2023

yihau commented Apr 20, 2023 • edited

yihau commented Apr 20, 2023 • edited

yihau left a comment

Choose a reason for hiding this comment

Xuanwo commented Apr 20, 2023

Xuanwo left a comment

Choose a reason for hiding this comment

ryoqun commented Apr 20, 2023

Xuanwo commented Apr 20, 2023

sylvestre Apr 21, 2023

Choose a reason for hiding this comment

yihau Apr 21, 2023 • edited

Choose a reason for hiding this comment

sylvestre Apr 21, 2023

Choose a reason for hiding this comment

yihau Apr 21, 2023

Choose a reason for hiding this comment

sylvestre commented Apr 21, 2023 via email

yihau commented Apr 21, 2023

yihau commented Apr 21, 2023

sylvestre commented Apr 21, 2023

Xuanwo commented Apr 24, 2023

ryoqun commented Apr 25, 2023

Xuanwo commented Apr 25, 2023

yihau commented Apr 20, 2023 •

edited

codecov-commenter commented Apr 20, 2023 •

edited

yihau commented Apr 20, 2023 •

edited

yihau commented Apr 20, 2023 •

edited

yihau Apr 21, 2023 •

edited