icc code coverage support #247

chinqw · 2017-01-25T02:33:17Z

No description provided.

Add icc compiler support

jimhester

I did a first pass review of the PR. It would be nice to test this on travis as well before merging. https://github.com/nemequ/icc-travis has an example of using icc on travis, I can look into it further.

jimhester · 2017-01-26T15:04:35Z

DESCRIPTION

@@ -57,7 +57,7 @@ Suggests:
    xml2 (>= 1.0.0),
    parallel,
    memoise
-License: MIT + file LICENSE
+License: MIT + file LICENSE + Oracle Contribution License


This should be License: MIT + file LICENSE | GPL-3

jimhester · 2017-01-26T15:06:58Z

Oracle Contribution under GPL-3

@@ -0,0 +1,18 @@
+This software (the "Software") is contributed under the terms of the GPL-3 with additional permission for MIT under GPL-3 Section 7 as follows: 


I think this license information should go in inst/COPYING.

jimhester · 2017-01-26T15:07:23Z

R/compiled.R

@@ -99,3 +99,152 @@ run_gcov <- function(path, quiet = TRUE,
      class = "coverage")
  })
 }
+


This code should go in R/icc.R.

jimhester · 2017-01-26T15:19:52Z

R/compiled.R

+# parse icc compiler code coverage output
+parse_icov <- function(lines, package_path = "") {
+
+  source_file <- trimws(lines[1L])


trimws is only available in (R >= 3.3.0), covr supports (R >= 3.1.0), so an alternative would need to be provided for earlier versions.

jimhester · 2017-01-26T15:22:42Z

R/compiled.R

+    capture(name = "idcol", digits), "\t", any_spaces,
+    capture(name = "coverage", digits))
+
+  showCfunctions <- getOption("covr.showCfunctions", FALSE)


This should follow the conventions in the rest of the package and be snake_case.

jimhester · 2017-01-26T16:09:46Z

R/covr.R

+                rex::rex(start, "CC = ",
+                         capture(name = "compiler", anything),
+                         " ", anything))$compiler
+  compiler <- compiler[!is.na(compiler)]


We should error if no compiler is found.

jimhester · 2017-01-26T16:12:21Z

R/covr.R

+                         " ", anything))$compiler
+  compiler <- compiler[!is.na(compiler)]
+
+  res <- try(system(paste(compiler, "--version"), intern=TRUE), silent=TRUE)


tryCatch( system2(compiler, "--version"), error = function(e) { warning("cannot recognize a supported compiler") })

jimhester · 2017-01-26T16:13:05Z

R/covr.R

+      return("gcc")
+    else if (length(grep("icc ", res)) > 0L)
+      return("icc")
+  }


What about clang? Typically it is aliased as gcc, but it can also be explicitly specified.

jimhester · 2017-01-26T16:17:18Z

R/compiled.R

+
+  icov_inputs <- list.files(path, pattern = rex::rex(".dyn", end),
+                            recursive = TRUE, full.names = TRUE)
+


If there are no icov inputs we can probably return early here.

jimhester · 2017-01-26T16:17:54Z

R/compiled.R

+  icov_inputs <- list.files(path, pattern = rex::rex(".dyn", end),
+                            recursive = TRUE, full.names = TRUE)
+
+  icov_profmerge <- getOption("covr.icov_prof", "")


Return if this is not set?

chinqw

edited the changes based upon the comments.
Will move parse_icov and run_icov to a new file icc.R in a separate pull afterwards.

chinqw · 2017-03-29T00:10:49Z

Make changes according to the review comments and update DESCRIPTION and LICENSE files with new proposal. The new license proposal "License: MIT + file LICENSE + Portions GPLv3 with Additional Permission | GPLv3" in DESCRIPTION file seems to cause R CMD check a warning which is flagged by travis.

chinqw · 2017-03-29T00:13:11Z

made changes according to review comments and updated license with a new proposal. It seems that the new license proposal 'License: MIT + file LICENSE + Portions GPLv3 with Additional Permission | GPLv3' causes R CMD check a warning which is flagged by travis.

CLAassistant · 2017-04-28T11:16:20Z

All committers have signed the CLA.

jimhester · 2017-04-28T11:22:16Z

I recently modified the covr license to be GPL-3 and you will need to sign the project CLA and remove the Oracle copyright clauses in order to contribute this code.

codecov-io · 2017-06-11T18:47:50Z

Codecov Report

Merging #247 into master will decrease coverage by 5.94%.
The diff coverage is 23.84%.

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
- Coverage   80.65%   74.71%   -5.95%     
==========================================
  Files          23       24       +1     
  Lines        1370     1499     +129     
==========================================
+ Hits         1105     1120      +15     
- Misses        265      379     +114

Impacted Files	Coverage Δ
R/icc.R	`0% <0%> (ø)`
R/zzz.R	`0% <0%> (ø)`	⬆️
R/covr.R	`88.93% <69.23%> (-2.62%)`	⬇️
R/compiled.R	`94.02% <94.73%> (+0.09%)`	⬆️

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 580c2fc...7be54d6. Read the comment docs.

chinqw · 2017-06-11T18:58:04Z

oracle license statement has been removed. Changes are sync'ed with master branch.

jimhester

Thank you for removing the comments, I have a few minor suggestions around detecting the compiler used.

Also could you add a note to NEWS.md with the change and mentioning yourself and add your name as a contributor to the DESCRIPTION file (if it is not already there).

jimhester · 2017-06-12T14:24:13Z

R/covr.R

+      relative = relative_path)
+  } else {
+  # use icc compiler
+    coverage <- structure(c(coverage, run_icov(pkg$path, quiet = quiet)),


Don't duplicate all of the code, just the object that differs.

res <- if (uses_icc()) { run_icov(pkg$path, quiet = quiet) } else { run_gcov(pkg$path, quiet = quiet) } coverage <- structure(c(coverage, res), class = "coverage", package = pkg, relative = relative_path)

jimhester · 2017-06-12T14:24:20Z

R/covr.R

@@ -429,3 +455,23 @@ add_hooks <- function(pkg_name, lib, fix_mcexit = FALSE) {

  writeLines(text = lines, con = load_script)
 }
+
+# check if gcc or icc is used
+get_compiler <- function() {


I think this function should just check if icc is used and fallback to the current behavior otherwise. Also you cannot assume the R CMD config CC will have trailing spaces, using the word boundary \\b anchor is a better solution.

uses_icc <- function() { compiler <- tryCatch( paste(system(paste(R.home("bin"), "R --vanilla CMD config CC", sep="/"), intern = TRUE), collapse=""), error = function(e) "") grepl("\\bicc\\b", compiler) }

chinqw · 2017-06-17T01:55:27Z

suggested changes are done.

chinqw added 5 commits January 24, 2017 20:34

Update covr.R

75d4b48

Add icc compiler support

add icc compiler code coverage support

e30eaef

add icc code coverage settings

772d01b

Update License

d33f576

Oracle Contribution under GPL-3 License File

b4dffa2

jimhester requested changes Jan 26, 2017

View reviewed changes

chinqw added 4 commits March 17, 2017 23:04

add helper function line_coverages

4cc94b7

update get_compiler

5b59b2c

Update covr.R

4d9bd1d

Update compiled.R

94ed7f9

chinqw commented Mar 18, 2017

View reviewed changes

chinqw added 7 commits March 18, 2017 00:49

exclude windows from icc

edf05dc

remove unnecessary path check from run_icov

1bf9da6

update License in DESCRIPTION file

6ee6904

Delete Oracle Contribution under GPL-3

75c754a

Update LICENSE

c5c04fc

Create icc.R

55f2b4f

Update compiled.R

a7a2e11

chinqw closed this Mar 29, 2017

chinqw reopened this Mar 29, 2017

jimhester mentioned this pull request Mar 31, 2017

Change of covr license to GPL-3 #256

Closed

20 tasks

jimhester closed this Apr 28, 2017

jimhester reopened this Apr 28, 2017

chinqw added 3 commits June 11, 2017 11:55

remove changes to License file

1adb860

remove oracle terms from comments

cb5ab55

remove oracle term from comments

6206fe5

chinqw added 3 commits June 11, 2017 12:06

remove oracle term from comments

2f27d07

rollback change

e967534

Merge branch 'master' into patch-1

0119362

jimhester reviewed Jun 12, 2017

View reviewed changes

chinqw added 6 commits June 15, 2017 21:45

add new contributor

344e78a

add icc code coverage support in NEWS

1d11880

update icc call

db6da9f

Update covr.R

77e758d

Update DESCRIPTION

3dec615

Update DESCRIPTION

7be54d6

jimhester approved these changes Jun 23, 2017

View reviewed changes

jimhester merged commit 7be54d6 into r-lib:master Jun 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

icc code coverage support #247

icc code coverage support #247

chinqw commented Jan 25, 2017

jimhester left a comment

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

jimhester Jan 26, 2017

chinqw left a comment

chinqw commented Mar 29, 2017

chinqw commented Mar 29, 2017

CLAassistant commented Apr 28, 2017 •

edited

jimhester commented Apr 28, 2017

codecov-io commented Jun 11, 2017 •

edited

chinqw commented Jun 11, 2017

jimhester left a comment

jimhester Jun 12, 2017

jimhester Jun 12, 2017

chinqw commented Jun 17, 2017

		@@ -0,0 +1,18 @@
		This software (the "Software") is contributed under the terms of the GPL-3 with additional permission for MIT under GPL-3 Section 7 as follows:


		icov_inputs <- list.files(path, pattern = rex::rex(".dyn", end),
		recursive = TRUE, full.names = TRUE)

icc code coverage support #247

icc code coverage support #247

Conversation

chinqw commented Jan 25, 2017

jimhester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinqw left a comment

Choose a reason for hiding this comment

chinqw commented Mar 29, 2017

chinqw commented Mar 29, 2017

CLAassistant commented Apr 28, 2017 • edited

jimhester commented Apr 28, 2017

codecov-io commented Jun 11, 2017 • edited

Codecov Report

chinqw commented Jun 11, 2017

jimhester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinqw commented Jun 17, 2017

CLAassistant commented Apr 28, 2017 •

edited

codecov-io commented Jun 11, 2017 •

edited