Skip to content

Conversation

@jolars
Copy link
Contributor

@jolars jolars commented Sep 2, 2017

This pull request updates RcppParallel to use Intel TBB 2017 Update 7. The motivation is both to take advantage of the improvements in TBB and to hopefully fix some issues in RcppParallel. I have also updated the version number to 2017.7.0 to reflect the changed version pattern in TBB and the readme to cover the change of license (from GPL to Apache).

The package has been tested for release, devel, and oldrel for linux (gcc and clang) and release and devel on osx on travis-ci and windows locally, where it checks out okay. There are failures on appveyor, however, that I haven't been able to crack. Solaris has not been tested.

I am sort of out of my depth here so it would be fantastic is someone more proficient could chip in to review this.

@eddelbuettel
Copy link
Member

If it helped, I could probably run a rev.dep check for this. Per the [RcppParallel] CRAN page(https://cloud.r-project.org/web/packages/RcppParallel/index.html), it is pretty short list of reverse depends.

@jolars
Copy link
Contributor Author

jolars commented Sep 2, 2017

Thank you, but I already did here. There is an error with catSurv that I'm looking in to.

src/Makevars Outdated
# a possible '-m64' attached to the CC variable
# (or other compiler arguments)
PKG_CPPFLAGS += $(CXX1XSTD) -I../inst/include/
PKG_CPPFLAGS += $(CXX11STD) -I../inst/include/
Copy link
Contributor

Choose a reason for hiding this comment

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

Does R 3.0.2 understand CXX11STD? (Just because we in theory support R back to that version)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I thought that "CXX1X" had been deprecated in favor of "CXX11"?

From the manual:

R versions 3.1.0 to 3.3.3 used CXX1X rather than CXX11, and these forms are deprecated but still accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed it's been deprecated, but because RcppParallel currently requires R >= 3.0.2, I just wanted to make sure that the package still builds successfully there.

Alternatively, we can just bump the minimum R version in the DESCRIPTION to R 3.1.0?

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the CXX1X != CXX11 change done in R 3.4.0? IIRC it still understands the older form but prefers the newer.

@kevinushey
Copy link
Contributor

@eddelbuettel what's your opinion on including the revdep folder as part of the RcppParallel GitHub repository?

@jjallaire
Copy link
Member

jjallaire commented Sep 2, 2017 via email

@eddelbuettel
Copy link
Member

eddelbuettel commented Sep 2, 2017

What folder?

For rev.deps I run I keep the logs in this repo. But that is just how my workflow evolved so it doesn't really matter.

@jolars
Copy link
Contributor Author

jolars commented Sep 2, 2017

I tried my best to track down and preserve all the fixes and modifications for Solaris and Windows but it would be great if someone could double check that it works.

The revdep checks now all check out. It turns out I hadn't set up GSL properly.

@kevinushey
Copy link
Contributor

I'll try building on my Windows VM as well just to double-check that everything is in order.

PS: thanks for taking the time to put together this PR, updating TBB here is long overdue!

@kevinushey
Copy link
Contributor

I tried building this branch with R 3.2.5 and it failed due to CXX11 and CXX11STD not being defined. Can we stick with CXX1X and CXX1XSTD?

@jolars
Copy link
Contributor Author

jolars commented Sep 3, 2017

It works on win-builder.

(I had to temporarily hijack the maintainer field -- hope that's fine).

@eddelbuettel
Copy link
Member

eddelbuettel commented Sep 5, 2017

This looks like a nicely done job -- thanks Johan!

I am technically not a maintainre or author here on this repo so I shouldn't do the merge but someone may pull the trigger...

@jjallaire jjallaire merged commit f597d4b into RcppCore:master Sep 5, 2017
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