GH-40343: [C++] Move S3FileSystem to the registry

[bkietz]

Rationale for this change

Moving S3 to a separate module will allow separate build the module for use by builds of arrow which did not include S3 funcionality.

What changes are included in this PR?

• the factory for S3 filesystems is added to the registry
• an internal function is added which unregisters functions for testing purposes

Are these changes tested?

A test for just the modularization is added which unregisters built in factories. This way we're definitely testing loading new factories from module libraries.

Are there any user-facing changes?

No

• GitHub Issue: [C++] Move S3FileSystem to a separate module #40343

[wgtmac]

Why do we need this?

[bkietz]

This is used in GetOrcMajorVersion(), so it should be #included here by IWYU. I found it because compilation failed at one point (compilation can spuriously succeed when standard headers include more definitions than the standard guarantees; for example, inclusion of only <string> or <iostream> might fully define std::stringstream rather than only forward declaring it)

[felipecrv]

Do we remove the linking of the main arrow library against AWSSDK_LINK_LIBRARIES when ARROW_S3_MODULE is ON?

[bkietz]

We don't want to require the main arrow library to load the module. After the module can reproduce the full functionality of the S3FileSystem (S3ProxyOptions, for example) we can deprecate building s3 in the main library. That's not in scope for now, though

[felipecrv]

@assignUser does the auto-formatter for CMake has the ability to indent these?

                   SOURCES
                     s3fs_module_test.cc
                     s3_test_util.cc
                   EXTRA_LABELS
                     filesystem
                   DEFINITIONS
                     ARROW_S3_LIBPATH="$<TARGET_FILE:arrow_s3fs>"
                   EXTRA_LINK_LIBS
                     Boost::filesystem
                     Boost::system)

[bkietz]

It looks like we could, but it'd involve declaring each function like add_arrow_library to additional_commands https://cmake-format.readthedocs.io/en/latest/configuration.html#configuration

[assignUser]

Sorry, missed that ping. But yes, that is possible as Ben says, I have done something similar for velox custom functions. But currently I am a little concerned that cmake-format seems to be unmaintained https://github.com/cheshirekow/cmake_format/issues/340

[felipecrv]

Should these be in URIs? Has this always been the case?

[bkietz]

This has been in place since S3 URIs were supported, #6403

The URI is intended to be a fully self contained initializer for a filesystem, so if the filesystem requires secrets for initialization then the URI must contain them

[assignUser]

Is this still recommended practice? It seems Azure recommends not doing this to prevent accidental exposure of credentials in logs etc. https://lists.apache.org/thread/mhwr2lvtxvjcqos12k7hr4cqkdofrxxo

I don't know anything about S3 auth though so 🤷 (in any case probably something for a follow up)

[felipecrv]

@pitrou do you want to merge this one?