Skip to content
This repository was archived by the owner on Jan 15, 2024. It is now read-only.

Conversation

@nealrichardson
Copy link
Contributor

@nealrichardson nealrichardson commented Oct 15, 2020

Changes:

  • S3 enabled and aws-sdk-cpp added as a dependency (this will not be added to rtools-backports)
  • Mimalloc enabled (the Vista compatibility patch has been released)
  • Add some new USE_SHARED=OFF flags

@jeroen
Copy link
Contributor

jeroen commented Oct 15, 2020

If you use -DARROW_SNAPPY_USE_SHARED=OFF can you remove mingw-w64-snappy from the dependencies? Perhaps also other dependencies that you no longer use?

@nealrichardson
Copy link
Contributor Author

Those _USE_SHARED=OFF flags just say to look for the static version of those dependencies, not that they're not dependencies anymore.

@jeroen
Copy link
Contributor

jeroen commented Oct 15, 2020

Are you sure? Typically SHARED=OFF means to build a vendored version of the library.

@nealrichardson
Copy link
Contributor Author

We govern that by the DEPENDENCY_SOURCE=BUNDLED|SYSTEM flags, and these are all SYSTEM. You can see in the build logs that there are no e.g. snappy_ep or other cmake external projects being built, they're coming from the system packages.

These USE_SHARED flags came from this commit, which explains what they're for: apache/arrow@81d3f26

@jeroen
Copy link
Contributor

jeroen commented Oct 17, 2020

By the way, does this version of arrow assume hw features on the machine of the user, or does this get detected at runtime?

The CRAN winbuilder runs very old hardware, there are some other packages that crash on CRAN Winbuilder because they enable SIMD features that may be supported in the rtools40 compilers, but actually not available on the CRAN hardware (at least I think that is the problem): https://cran.r-project.org/web/packages/RcppXsimd/index.html

@nealrichardson
Copy link
Contributor Author

AVX512 is disabled on mingw always: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/SetupCxxFlags.cmake#L62

So the inaccurate SIMD detection we patched in autobrew won't matter here.

@nealrichardson nealrichardson marked this pull request as ready for review October 19, 2020 20:49
@jeroen jeroen merged commit b444044 into r-windows:master Oct 19, 2020
@nealrichardson nealrichardson deleted the patch-6 branch October 19, 2020 22:33
@jeroen
Copy link
Contributor

jeroen commented Oct 19, 2020

Thanks! All seems to work but I recommend you have a look at the -Wmaybe-uninitialized compiler warnings shown in the CI.

@nealrichardson
Copy link
Contributor Author

Thanks. We need to set up better CI for that, I feel like I find more of those warnings every time I look.

@jeroen
Copy link
Contributor

jeroen commented Oct 21, 2020

You just need to add -Werror to CXXFLAGS in your CI, then it will fail for compiler warnings.

@nealrichardson
Copy link
Contributor Author

I thought I had already done that though. Maybe I only set it for the R bindings, not the C++ library build? In any case, thanks, will take care of these.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants