-
Notifications
You must be signed in to change notification settings - Fork 185
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
Improve the MinGW builds and simplify bundling the static library dependencies. #4448
Conversation
It now flows through the superbuild and gets respected when overridden in the command line.
Dependencies provided by MinGW packages are not placed in the `vcpkg_installed` folder, requiring logic in the build script to have them bundled, which logic we want to remove.
This pull request has been linked to Shortcut Story #35901: Copy the vcpkg_installed directory to the MinGW package.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as @eddelbuettel confirms this, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I abstain, your honor."
I have no Windows system to test. I could test this if the artifacts from the PR were piled up somewhere where I could access them. I could then try modify the build and overwrite the once we download by default, and use these.
That said, given the recent test under 2.18.0-rc0 I am confident we can make this work. I just cannot assert that this works.
You could make a branch in |
Might be easier to just borrow a few minutes from a colleague using Windows and R, and I have a "volunteer" in mind. Does the PR produce artifacts someone can access? |
Sweet, thanks. I missed that. There are too many builds triggered by you :) so I missed this. |
@mojaveazure was kind enough to give it a whirl but seems to have uncovered that this build was up to date with some recent merges in dev and some of the enumerations extensions we need in the current version of the R package are missing -- so a build attempt failed. Another try with the released version of the R package should shed more light. |
@teo-tsirpanis : There is a lot of 'shrapnel' @mojaveazure is seeing in AWS linkage. Example |
2.18 has updated the AWS SDK and the new version unconditionally calls You will have to update the linked libraries in
|
We will try to work it out. On Linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Group approving" as @mojaveazure @teo-tsirpanis and I were able to ascertain that the artifacts produced allow the R packages both from the most recent release as well as from HEAD build, install, and test cleanly.
Includes updated linking directories and libraries for expanded/simplified mingw build of tiledb-core
SC-35901
As a preparation towards stopping bundling our static library dependencies (which would lead to significant simplifications in the build system), this PR updates the MinGW builds (which supply the binaries the R API uses on Windows) to stop relying on that automatic bundling. Our approach becomes more blunt, just copying the contents of the build directory's
vcpkg_installed
directory, where vcpkg places all libraries we might need.Nothing (fingers crossed) should change on the side of the R API. These are the libraries that are bundled before and after this PR:
Before
After
Notice that no libraries have stoppped from being bundled. Because we now bundle libraries we don't actually need, the package's size increased:
Also the package now includes some useless files like around one thousand headers for the AWS SDK, but they seem to be excluded from being uploaded.
I also made some improvement that should speed-up the MinGW builds (build dependencies in Release mode only, cache them and disable tests).
TYPE: IMPROVEMENT
DESC: Improve building the Windows binaries for the R API.