pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.18k stars 698 forks source link

make filesystem compatible with gcc7.5.0 and gcc >=8 #1090

Closed zydxhs closed 2 years ago

zydxhs commented 2 years ago

This is a follow up to #1061, use has_include instead of ifdef

Tachi107 commented 2 years ago

Hi @zydxhs, thanks for reopening this! We've reverted #1061, so as you can see this PR has conflicts. Could you please rebase on the latest master?

I usually use the following commands:

git switch 408
git remote add upstream https://github.com/pistacheio/pistache.git
git fetch upstream
git rebase upstream/master
git push --force
zydxhs commented 2 years ago

@Tachi107 I've done it now.

codecov-commenter commented 2 years ago

Codecov Report

Merging #1090 (a47724b) into master (5bf1a89) will decrease coverage by 0.17%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
- Coverage   78.30%   78.13%   -0.18%     
==========================================
  Files          53       53              
  Lines        6657     6658       +1     
==========================================
- Hits         5213     5202      -11     
- Misses       1444     1456      +12     
Impacted Files Coverage Δ
src/common/description.cc 43.70% <100.00%> (+0.18%) :arrow_up:
include/pistache/transport.h 53.75% <0.00%> (-5.00%) :arrow_down:
src/common/transport.cc 66.99% <0.00%> (-1.66%) :arrow_down:
src/client/client.cc 84.06% <0.00%> (-0.32%) :arrow_down:
include/pistache/async.h 89.76% <0.00%> (-0.19%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kiplingw commented 2 years ago

@zydxhs, it looks like you are having a CI failure on linux / linux (registry.access.redhat.com/ubi8/ubi-minimal, clang, none). Looks harmless, but you should take a look. Once all CI has passed, if @Tachi107 is fine with it, we will merge.

zydxhs commented 2 years ago

@kiplingw There is still one CI failure, but I don't know why and how to resolve it. It seems like it depends on the compilation environment?

kiplingw commented 2 years ago

I was actually wondering that myself. This might be something only @Tachi107 can solve. It looks like it's a dependency problem with a deprecated version of the Python runtime.

Tachi107 commented 2 years ago

Yes, it seems that RHEL 8 broke their clang package recently. I've also encountered this issue in another software I maintain that has a similar CI environment.

I haven't investigated the issue yet, but it is not related to this PR in particular.

Anyway, I'll review this soon

kiplingw commented 2 years ago

@Tachi107, if you can rebase, re-trigger CI, and verify everything passes, you can go ahead and merge when ready.

Tachi107 commented 2 years ago

I'm looking at the diff right now

kiplingw commented 2 years ago

Nice work again @Tachi107.

Tachi107 commented 2 years ago

Thanks, @zydxhs!

I'll re-merge the changes you previously submitted in the master branch as soon as possible (or you can submit yet-another pull request if you prefer)