GH-41990: [C++] Fix AzureFileSystem compilation on Windows #48971
+43
−1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let me preface this pull request that I have not worked in C++ in quite a while. Apologies if this is missing modern idioms or is an obtuse fix.
Rationale for this change
I encountered an issue trying to compile the AzureFileSystem backend in C++ on Windows. Searching the issue tracker, it appears this is already a known but unresolved problem. This is an attempt to either address the issue or move the conversation forward for someone more experienced.
What changes are included in this PR?
AzureFileSystem uses
unique_ptrwhile the other cloud file system implementations rely onshared_ptr. Since this is a forward-declared Impl in the headers file but the destructor was defined inline (via= default), we're getting compilation issues with MSVC due to it requiring the complete type earlier than GCC/Clang.This change removes the defaulted definition from the header file and moves it into the .cc file where we have a complete type.
Unrelated, I've also wrapped 2 exception variables in
ARROW_UNUSED. These are warnings treated as errors by MSVC at compile time. This was revealed in CI after resolving the issue above.Are these changes tested?
I've enabled building and running the test suite in GHA in 8dd62d6. I believe a large portion of those tests may be skipped though since Azurite isn't present from what I can see. I'm not tied to the GHA updates being included in the PR, it's currently here for demonstration purposes. I noticed the other FS implementations are also not built and tested on Windows.
One quirk of this PR is getting WIL in place to compile the Azure C++ SDK was not intuitive for me. I've placed a dummy
wilConfig.cmaketo get the Azure SDK to build, but I'd assume there's a better way to do this. I'm happy to refine the build setup if we choose to keep it.Are there any user-facing changes?
Nothing here should affect user-facing code beyond fixing the compilation issues. If there are concerns for things I'm missing, I'm happy to discuss those.