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

Help find and troubleshoot pprof installation #15

Merged
merged 9 commits into from
Feb 25, 2020
Merged

Help find and troubleshoot pprof installation #15

merged 9 commits into from
Feb 25, 2020

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented Feb 6, 2020

Summary

To address #13 and #14, this PR exports 3 functions:

  • pprof_path(): return the path to pprof if it exists and "" otherwise.
  • assert_pprof(): error if pprof is not found.
  • test_pprof(): tests the pprof() function on a small example to make sure the pprof installation is usable.

By default, each function prints console messages whenever it struggles to find pprof. Set verbose = FALSE to turn these messages off.

Related GitHub issues and pull requests

Checklist

  • I understand and agree to proffer's code of conduct.
  • I have listed any substantial changes in the development news.
  • I have added testthat unit tests to tests/testthat for any new functionality. Feel free to use skip("interactive only") for any tests that cannot be fully automated.
  • This pull request is not a draft.

@wlandau wlandau requested a review from krlmlr February 6, 2020 17:27
@wlandau wlandau self-assigned this Feb 6, 2020
@wlandau
Copy link
Member Author

wlandau commented Feb 6, 2020

pprof_path() still does not work on Windows. I need to investigate when I get back to a Windows machine.

@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #15 into master will decrease coverage by 3.89%.
The diff coverage is 47.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #15     +/-   ##
=========================================
- Coverage   46.42%   42.52%   -3.9%     
=========================================
  Files           3        4      +1     
  Lines          56       87     +31     
=========================================
+ Hits           26       37     +11     
- Misses         30       50     +20
Impacted Files Coverage Δ
R/pprof.R 0% <0%> (-25%) ⬇️
R/utils.R 100% <100%> (ø) ⬆️
R/troubleshoot.R 45.94% <45.94%> (ø)

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 85f4e60...a769b45. Read the comment docs.

@wlandau
Copy link
Member Author

wlandau commented Feb 7, 2020

As of f76bde5, proffer searches for pprof on Windows too (if pprof_path is not set). So I think this PR completely solves #14. Maybe #13 as well, unless we want an all-in-one check for future system dependencies we may add in addition to pprof.

@wlandau
Copy link
Member Author

wlandau commented Feb 20, 2020

@krlmlr, what do you think about these new functions for proffer?

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks.

We can simplify the Windows installation a bit if we look at default locations for the executables if they're not on the $PATH. This seems possible for Go and doesn't look too difficult for Graphviz. Also, we could provide an installation helper that installs {RProtoBuf} and directs the user to installers for Windows, or tells which packages to install on Linux and macOS.

I'll take a stab.

README.Rmd Outdated Show resolved Hide resolved
README.Rmd Outdated Show resolved Hide resolved
wlandau-lilly and others added 2 commits February 25, 2020 10:30
Co-Authored-By: Kirill Müller <[email protected]>
Co-Authored-By: Kirill Müller <[email protected]>
@wlandau wlandau merged commit 6e2fea9 into master Feb 25, 2020
@wlandau wlandau deleted the 14 branch February 25, 2020 16:05
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.

4 participants