Skip to content
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

Update instrument-coverage.md #105954

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Update instrument-coverage.md #105954

merged 1 commit into from
Jan 16, 2023

Conversation

gftea
Copy link
Contributor

@gftea gftea commented Dec 20, 2022

explicitly set environment variable LLVM_PROFILE_FILE="default_%m.profraw" when starting cargo test, otherwise, only one default.profraw is generated and will be overwritten if run unnittests and integration tests at the same time.

@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2022
@ehuss
Copy link
Contributor

ehuss commented Dec 20, 2022

Thanks! Can you say more about under which circumstances it will only generate a single file? My understanding is that the default will disambiguate each file such as default_11566440541585291497_0_48944.profraw. The default should be set here.

@gftea
Copy link
Contributor Author

gftea commented Dec 20, 2022

my local env has rustc 1.63, and I run the tests with below commands

RUSTFLAGS="-C instrument-coverage -C codegen-units=1" cargo test -p amqprs --all-features --tests

the integration tests overwrite the unittests data, so read the coverage data using object file of unittests, it shows all zeros.

Once I set LLVM_PROFILE_FILE="default_%m.profraw" , then I get both coverage data for unittests and integration tests

@ehuss
Copy link
Contributor

ehuss commented Dec 20, 2022

I believe it was changed in 1.65 via #100384. Can you try with a newer version?

@gftea
Copy link
Contributor Author

gftea commented Dec 20, 2022

even it works in later rustc version, it is still important to have that information otherwise users follow the guideline will encounter issues

@ehuss
Copy link
Contributor

ehuss commented Dec 20, 2022

I think it would be great to make it a little clearer what the default is for LLVM_PROFILE_FILE if it is not specified. I don't think the documentation should encourage changing the default as a matter of course, as the default should be serviceable for most situations. The documentation already includes a note just below recommending using %m and %p. We generally don't document the behavior of older versions of the compiler, as it would eventually be overwhelming to have every part annotated with every change. However, in this situation, it might be worthwhile to mention that the default changed in 1.65. Perhaps something like "The default for LLVM_PROFILE_FILE is default_%m_%p.profraw (versions prior to 1.65 had a default of default.profraw)."

@gftea
Copy link
Contributor Author

gftea commented Dec 20, 2022

I agree with your points to be more specific, but for user on older verson than 1.65, it will cause incorrect behaviors without explicitly set the LLVM_PROFILE_FILE.
If user testing MSRV older than 1.65, then it will have false coverage reports according to guideline.
maybe we can add additonal info about the defaults, but I think it is better to have it explicitly set.

@ehuss
Copy link
Contributor

ehuss commented Dec 29, 2022

I disagree, as I think it is best for the default experience to not imply that LLVM_PROFILE_FILE is necessary. We generally don't provide instructions on how to use older versions of Rust. It is unfortunate that older versions of Rust did not provide a better default (and was not well documented). The suggested text I provided should provide enough information if someone is using an older version.

@gftea
Copy link
Contributor Author

gftea commented Jan 16, 2023

@ehuss , can you review the latest changes based on your suggestion

@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

Could not assign reviewer from: ehuss.
User(s) ehuss are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

Document the default for LLVM_PROFILE_FILE and add a recemmondation for setting
it for older versions of Rust which had a different default.
@ehuss
Copy link
Contributor

ehuss commented Jan 16, 2023

I posted some edits to clarify some of the wording and to use a consistent formatting. I also squashed the commits.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 16, 2023

📌 Commit c1f1f60 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2023
@gftea
Copy link
Contributor Author

gftea commented Jan 16, 2023

I posted some edits to clarify some of the wording and to use a consistent formatting. I also squashed the commits.

thanks!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#105954 (Update instrument-coverage.md)
 - rust-lang#106835 (new trait solver: rebase impl substs for gats correctly)
 - rust-lang#106912 (check -Z query-dep-graph is enabled if -Z dump-dep-graph (rust-lang#106736))
 - rust-lang#106940 (Improve a TAIT error and add an error code plus documentation)
 - rust-lang#106942 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cf533dd into rust-lang:master Jan 16, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 16, 2023
@gftea gftea deleted the patch-1 branch January 17, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants