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

Expanded the docs from comments in GRCOV-249 #387

Merged
merged 11 commits into from
Mar 17, 2020

Conversation

gilescope
Copy link
Contributor

No description provided.

@gilescope gilescope changed the title Exapanded the documentation from comments in GRCOV-249 Expanded the docs from comments in GRCOV-249 Feb 15, 2020
@gilescope
Copy link
Contributor Author

Fixes #249

@codecov-io
Copy link

codecov-io commented Feb 15, 2020

Codecov Report

Merging #387 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #387   +/-   ##
=======================================
  Coverage   65.27%   65.27%           
=======================================
  Files          12       12           
  Lines        4101     4101           
  Branches     1033     1033           
=======================================
  Hits         2677     2677           
  Misses        622      622           
  Partials      802      802           

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 c8d7e69...83ae8be. Read the comment docs.

@gilescope
Copy link
Contributor Author

Some pretty epic coverage swings considering it's just the readme that's changed??

@Nokel81
Copy link

Nokel81 commented Feb 15, 2020

Would you be able to add a little blurb on each of the -t options? (In a list format or something). I think that new users would fine it quite useful.

@gilescope gilescope force-pushed the more-docs branch 8 times, most recently from e6b02cf to 33582d7 Compare February 15, 2020 17:54
@gilescope
Copy link
Contributor Author

@Nokel81 how's that looking? Is there any way we can improve it more (short of writing a cargo-grcov crate)?

@Nokel81
Copy link

Nokel81 commented Feb 16, 2020

Last I checked grcov doesn't work on macos. But other then that this looks good.

@gilescope
Copy link
Contributor Author

Luckily I didn't scrub OSX when I switched to nixos. I'm going to stand my ground here - just checked OSX, works fine for a small project. I'm not saying there might not be bugs on OSX/Windows but certainly for some projects it works on all three major OSes.

README.md Outdated Show resolved Hide resolved
@gilescope
Copy link
Contributor Author

(And btw that's one of the reason's I'm a big fan. As far as I know grcov is the only rust coverage mechanism that work on all three OSes.)

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@Nokel81
Copy link

Nokel81 commented Feb 17, 2020

I was referring to rust-lang/rust#63047 since this is what I ran into.

@gilescope gilescope force-pushed the more-docs branch 2 times, most recently from e36131a to 56a0716 Compare February 18, 2020 08:25
@gilescope
Copy link
Contributor Author

@Nokel81 I guess we don't know how many are using grcov on mac with no problems? Is there a set of rustc flags that did work for you? - if so we could let people know those alternative settings should people run into trouble.

@Nokel81
Copy link

Nokel81 commented Feb 18, 2020

I didn't, I switched to running in Docker containers...

@gilescope
Copy link
Contributor Author

Is the PR an improvement or not on the current docs? If so let's ship it and give users an easier time. If not please feel free to go in and change the PR to tweak things to how you want it or suggest what I can change - I'd like to get closure on this PR.

@gilescope
Copy link
Contributor Author

(Sorry just juggling a lot of things at the moment. I just want there to be better docs)

@Nokel81
Copy link

Nokel81 commented Feb 19, 2020

:shipit: (sorry for the blocking style comments, I just know that as a user on macos I would be confused if the project says it works on macos but then I run into such a linking error)

I agree that these doc edits will help users

@gilescope
Copy link
Contributor Author

We could categorise the open issues by OS and provide links to the open issues by operating system. But we cannot say it doesn’t work on macos if there are counterexamples.

We could confidently say it works on macos for some definition of ‘works’ and dependent on your codebase. :-)

(We should probably include windows in that definition of working... I have had some issues there)

Sent with GitHawk

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@calixteman
Copy link
Collaborator

Maybe we should add a word about the fact that gcov (with the correct version) is required when the build has been made with gcc but we've an internal parser for gcno/gcda coming from llvm (clang/rustc).

@gilescope
Copy link
Contributor Author

Rebased and incorporated feedback. Anything left to change?

Copy link
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@marco-c marco-c merged commit f6ec602 into mozilla:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants