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

Linux distros jobs on PRs + Fix linking issues on some platforms #2101

Merged
merged 2 commits into from
Feb 18, 2022

Conversation

piponazo
Copy link
Collaborator

Fixes #2100

@piponazo piponazo requested a review from hassec February 15, 2022 12:53
@piponazo piponazo self-assigned this Feb 15, 2022
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #2101 (8cd8b3a) into main (287744f) will not change coverage.
The diff coverage is n/a.

❗ Current head 8cd8b3a differs from pull request most recent head b7d5c7e. Consider uploading reports for the commit b7d5c7e to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2101   +/-   ##
=======================================
  Coverage   63.75%   63.75%           
=======================================
  Files          96       96           
  Lines       19202    19202           
  Branches     9798     9798           
=======================================
  Hits        12242    12242           
  Misses       4658     4658           
  Partials     2302     2302           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 287744f...b7d5c7e. Read the comment docs.

@neheb
Copy link
Collaborator

neheb commented Feb 16, 2022

Not quite correct.

For GCC 8 and clang 7-8, this is needed. GCC9 and clang9 and above do not. Well, the libc++ versions actually.

GCC7 supports std::experimental::filesystem which is basically boost::filesystem. I wouldn't bother with it though.

@piponazo
Copy link
Collaborator Author

Thanks for the detailed review! I'll be more explicit about those checks in the CMake code then.

@neheb
Copy link
Collaborator

neheb commented Feb 16, 2022

This is how I do it elsewhere: gerbera/gerbera@143217b#diff-30d8f6be6320feeacf686be94f48c70869b52630e01ea625f0f15adc0d57c3e4R16
It's not cmake but it's easy to translate I think.

@piponazo
Copy link
Collaborator Author

This is how I do it elsewhere: gerbera/gerbera@143217b#diff-30d8f6be6320feeacf686be94f48c70869b52630e01ea625f0f15adc0d57c3e4R16
It's not cmake but it's easy to translate I think.

Just for the sake of sharing: There is a long discussion about this topic in a CMake gitlab issue:
https://gitlab.kitware.com/cmake/cmake/-/issues/17834

It seems difficult to come up with something very robust (for all possible cases). I'll try to come up with something good enough for the platforms we support in our CI infrastructure.

@neheb
Copy link
Collaborator

neheb commented Feb 17, 2022

I forgot to mention one thing. On 32-bit OS, GCC 8.3 has a bug where std::filesystem does not work with larger than 2GB files. This was fixed with 8.4 I don't know if that's relevant. Debian 10 comes with GCC 8.3.

@piponazo
Copy link
Collaborator Author

I forgot to mention one thing. On 32-bit OS, GCC 8.3 has a bug where std::filesystem does not work with larger than 2GB files. This was fixed with 8.4 I don't know if that's relevant. Debian 10 comes with GCC 8.3.

Definitely it is good to know. I'm adding this link for future reference in case this bites us in the future:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91947

cmake/FindFilesystem.cmake Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger Linux-Distributions jobs on PRs
3 participants