Skip to content

check if R CMD SHLIB can compile OpenMP directly - #4374

Merged
mattdowle merged 4 commits into
Rdatatable:masterfrom
kevinushey:feature/macos-openmp-detection
Jun 9, 2020
Merged

check if R CMD SHLIB can compile OpenMP directly#4374
mattdowle merged 4 commits into
Rdatatable:masterfrom
kevinushey:feature/macos-openmp-detection

Conversation

@kevinushey

@kevinushey kevinushey commented Apr 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Part of #4348.

The intention here is that if the user has already modified src/Makevars such that OpenMP "just works", then that should be detected by explicitly validating that R CMD SHLIB can compile with OpenMP.

This allows data.table to compile using OpenMP for me when using the following Makevars:

CPPFLAGS     += -I/usr/local/opt/libomp/include -Xpreprocessor -fopenmp
LDFLAGS      += -L/usr/local/opt/libomp/lib -lomp

SHLIB_OPENMP_CFLAGS   =
SHLIB_OPENMP_CXXFLAGS =

Asking whether we can set those flags (or similar) in R 4.0.0 is a separate question, I think.

@mattfidler mattfidler mentioned this pull request Apr 12, 2020
Closed
jangorecki
jangorecki approved these changes Apr 12, 2020
View reviewed changes

@jangorecki jangorecki left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank @kevinushey for this PR.
Works fine on my local Xubuntu.
GitLab pipeline (unfortunately we don't have Mac build there yet #3326) revealed that clean up is not working good enough:

* checking if this is a source package ... NOTE
 Found the following apparent object files/libraries:
   test-omp.o
 Object files/libraries should not be included in a source package.

* checking top-level files ... NOTE
 Non-standard file/directory found at top level:
   ‘test-omp.o’

@mattdowle could you please invite Kevin to our members group, so he can push directly to Rdatatable/data.table branches?

@codecov

codecov Bot commented Apr 12, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #4374 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4374   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files          73       73           
  Lines       14104    14104           
=======================================
  Hits        14049    14049           
  Misses         55       55

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 dc5e11a...3c28fa7. Read the comment docs.

@kevinushey kevinushey mentioned this pull request May 5, 2020
Closed
@jangorecki jangorecki added this to the 1.12.9 milestone Jun 3, 2020
@mattdowle

mattdowle commented Jun 9, 2020

Copy link
Copy Markdown
Member

Thanks, @kevinushey! As Jan suggested, I've invited you to be project member, and will add you to contributors list.

@mattdowle
mattdowle merged commit 65f0516 into Rdatatable:master Jun 9, 2020
mattdowle added a commit that referenced this pull request Jun 9, 2020
@mattdowle
Added Kevin to contributor list in DESCRIPTION and made the change he…
0cb9e95 
… suggested to pass checkbashisms; #4374
@jangorecki jangorecki mentioned this pull request Jun 10, 2020
Closed
@jangorecki jangorecki mentioned this pull request Jun 27, 2020
Closed
@mattdowle mattdowle mentioned this pull request Jul 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jangorecki jangorecki jangorecki approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.13.0

Development

Successfully merging this pull request may close these issues.

4 participants

@kevinushey @mattdowle @jangorecki @MichaelChirico