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

ARROW-9616: [C++][R] Remove templated base classes named /.*ConcurrencyWrapper/ #10914

Closed

Conversation

bkietz
Copy link
Member

@bkietz bkietz commented Aug 11, 2021

This is a guess; Windows assumptions about linkage + templated base classes have been problematic in the past

@bkietz bkietz requested a review from pitrou August 11, 2021 15:57
@github-actions
Copy link

@bkietz
Copy link
Member Author

bkietz commented Aug 11, 2021

shell: Rscript {0}
run: |
dir.create("~/.R")
writeLines(c("CPPFLAGS=-flto", "AR=gcc-ar", "NM=gcc-nm", "RANLIB=gcc-ranlib"), "~/.R/Makevars")
Copy link
Member

@pitrou pitrou Aug 12, 2021

Choose a reason for hiding this comment

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

Won't this make builds even slower?

@pitrou
Copy link
Member

pitrou commented Aug 12, 2021

I'm ok with the C++ changes in principle, though it doesn't seem to appease the R build.

@jeroen
Copy link
Contributor

jeroen commented Aug 12, 2021

@bkietz seems to have solved most errors, but still 1 remaining: https://gist.github.com/jeroen/0c8bbfcdc51c6ac6d0672a3ca164e532

Btw I am testing this with the new upcoming gcc-10 toolchain, I don't think it is needed to enable this in your CI with rtools4 at this point.

@bkietz
Copy link
Member Author

bkietz commented Aug 12, 2021

@pitrou it's actually a compiler error. Still a problem, though

@pitrou
Copy link
Member

pitrou commented Nov 8, 2021

What is the status on this, I'm assuming the underlying issue has been fixed otherwise?

@jeroen
Copy link
Contributor

jeroen commented Nov 8, 2021

No the issue still exists for arrow 6.0.0. The fix by @bkietz had fixed the problem for 90%, there was only 1 remaining lto error. However this is not very important, I use an experimental next-gen toolchain to test this.

@bkietz bkietz closed this Nov 9, 2021
@bkietz bkietz deleted the try-removing-ConcurrencyWrappers branch November 30, 2021 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants