Skip to content
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

Switch to gulrak/filesystem #938

Open
COM8 opened this issue Aug 3, 2023 · 2 comments
Open

Switch to gulrak/filesystem #938

COM8 opened this issue Aug 3, 2023 · 2 comments
Assignees
Milestone

Comments

@COM8
Copy link
Member

COM8 commented Aug 3, 2023

          Hey, Fabian, after some more research, I think I should update my recommendation here: 

I really think we should backport std::filesystem with https://github.com/gulrak/filesystem instead of boost::filesystem.

gulrak/filesystem is std::filesystem-compatible (and header-only), sidestepping a lot of the pain we were getting in my other project from the API differences between boost::filesystem and std::filesystem. I'm guessing it'll save you a lot of API-divergence pain, too, as code continues to build on filesystem.

What do you think?

Some notes:

  • I started by tossing up a PR with them Dynamic selection for more Apple platforms gulrak/filesystem#167 to add support for non-desktop Apple platforms (which we needed) and to clean things up a bit.
  • Since it's header-only, the change could be as easy as grabbing some of their headers to replace the contents of libcpr's filesystem.h.
    • I already hacked together something simple on the Bazel side--haven't pushed yet, but libcpr compiles cleanly if you just swap in gulrak/filesystem for boost, conditionals and all.
    • You guys almost certainly have comparative advantage over me with cmake, so I think I should ask if you'd do the actual integration there.
      [One tricky case to think about: If you want to support the shared-library case, you'll want to make sure the same filesystem importation gets picked even if the final binary has a higher min OS version (deployment target) than was used for libcpr compilation. Not sure how cmake usually handles this...
  • You might want to amend their conditionals further to use experimental/filesystem if you feel like it? Happy to add it to my PR, but I don't know the history or tradeoffs as well there as you might.

Cheers!
Chris

P.S. Wish I'd steered us to this straight away when I saw someone had added the broken boost backport...sorry I just learned about it.

Originally posted by @cpsauer in #929 (comment)

@COM8
Copy link
Member Author

COM8 commented Aug 3, 2023

@cpsauer sorry for the delayed response. I was on vacation.

Don't worry. This is how developing software works. It's an iterative process pushing it to a better and better state. Back then using boost::filesystem sounded like the right way to go and it worked (mostly) so I'm happy 😁.

To me https://github.com/gulrak/filesystem looks like a well maintained and by far more compatible replacement for std::filesystem so I also agree we should move to it sooner than later.

I can look into porting our code over to it soon. Probably in the next two weeks or since I'm catching up with stuff right now.

For now I created an issue for it, so we do not forget about it.

Thank you very much for looking into this and preparing everything required in gulrak/filesystem#167.

@COM8 COM8 self-assigned this Aug 3, 2023
@COM8 COM8 added this to the CPR 1.11.0 milestone Aug 3, 2023
@cpsauer
Copy link
Contributor

cpsauer commented Aug 3, 2023

np at all. hope you had a great one, and thanks for being great, as always, Fabian.

Since I wrote that, I've pushed up the wrapping of ghc::filesystem for bazel and switched things over on the Bazel side with a little patch. Lmk if you want me to help tactically on migrating here--though I should really still defer to your comparative advantage on cmake. Happy to spin up a draft PR that we could collaborate on--or you could give me access to wherever you're working on it?

A couple additional notes from things I learned that might speed things along:

  • We'll probably want to replace the calls to std::ofstream(path.c_str()) with just std::ofstream(path)--no need for that boost workaround on the Apple platforms this backport aims for (
    std::ofstream f(local_path_.c_str());
    ) from my 9450f8c
  • Header-only vs header-and-implementation
    • While the simplest thing would be to just copy in the contents of ghc::filesystem and replace the contents of filesystem.h with #include "ghc/fs_std.hpp", for Bazel I opted to use ghc::filesystem in non-header-only mode, #including "ghc/fs_std_fwd.hpp" from filesystem.h, and then creating a cpp file that compiles via #include "ghc/fs_std_impl.hpp". My thinking was: it would save users some compile time, Bazel makes it easy to grab remote code and have dependencies that aren't header only, and libcpr is already not header only. LMK what you think on the cmake side. I'm not sure how easy it is to do the equivalent grab-a-remote-dependency action in cmake.
    • Either way, I'll probably need to maintain a small patch for Bazel--unless you want to pull in my mini shim in the repo linked above, keeping the compile and inclusion of ghc filesystem out of this repo?

@COM8 COM8 modified the milestones: CPR 1.11.0, CPR 1.12.0 Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants