libcpr / cpr

C++ Requests: Curl for People, a spiritual port of Python Requests.
https://docs.libcpr.org/
Other
6.59k stars 937 forks source link

Improvements to CPR_USE_BOOST_FILESYSTEM #877

Closed cpsauer closed 1 year ago

cpsauer commented 1 year ago

Hey @COM8, @KingKili,

Thanks so much for all your work making this library great.

I just attempted an update the bazel build to libcpr v1.10, but things are going to need a tweak for most deployments to Apple platforms. This PR fixes the blocking issue.

In more detail:

Most builds for Apple platforms are reverse compatible with a number of past versions of the OS (typically "targeting" iOS 11.0 currently), leading to conditionally available APIs in libc++, which is bundled with the OS. Apple contributed pretty great mechanisms to clang for dealing with conditional availability (also being adopted in the next version of the Android NDK).

std::filesystem is one of these conditionally available APIs, being available in iOS 13 and macOS 10.15 and later. So we'll need a mechanism to conditionally fall back to boost::filesystem, even when the <filesystem> header is around.

812 attempted to resolve something related, I think, but enabling CPR_USE_BOOST_FILESYSTEM was actually a no-op for updated Apple platforms. The reason is that <filesystem> is available (with only conditionally available symbols), triggering the #include of std::filesystem before we get to the CPR_USE_BOOST_FILESYSTEM case.

This PR instead makes CPR_USE_BOOST_FILESYSTEM imperative. That is, if it's defined, boost::filesystem is always used, as one might expect, enabling Apple platforms users to fall back on Boost when needed, with failure being explicit rather than silent.

(While I was at it, I took a shot at unifying away the one code-level conditional on CPR_USE_BOOST_FILESYSTEM and fixed a typo. Note that, per the spec, std::ofstream(Path) is equivalent to localpath.c_str())

Thanks for your consideration and for all you do! Chris

(Tagging also @kpeo, the author of #812, since he laid the foundations and might have additional comments.) (For CMake, perhaps one might auto-enable CPR_USE_BOOST_FILESYSTEM based on the deployment target, but I'm not a currently a CMake user or expert, so I should probably explicitly hand that off and separate it from this PR.) (For GitHub searchability: the build errors users would see without CPR_USE_BOOST_FILESYSTEM would look like error: 'path' is unavailable: introduced in iOS 13.0 note: 'path' has been explicitly marked unavailable)

cpsauer commented 1 year ago

(test failure looks to be an unrelated network timeout or something? Differing failures between the previous commits w/ no relevant changes. Tried force pushing an identical branch (up to timestamps) just to rerun tests, but that's leading to a different set.)

COM8 commented 1 year ago

Sorry for the late response, but your PR managed to land just one day after I went on vacation for a month :D

cpsauer commented 1 year ago

Sweet! Thanks so much, @COM8. Moving back off my fork as we speak

Thanks for being so wonderfully responsive and a great owner--totally get it re vacation. I've been on the other end of that one with the tools I maintain. Hope you had a great break.

And thanks for the CI context for the future.

Cheers! Chris