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

Use gcc-10.2 instead of gcc-10.1 on CI, also fix one problem #2110

Merged

Conversation

alexezeder
Copy link
Contributor

@alexezeder alexezeder commented Jan 22, 2021

Somehow I missed that ubuntu-18.04 image on CI has GCC-10.1 installed, so CI is unable to build compile-time formatting tests because they require GCC-10.2+.

fmt/include/fmt/format.h

Lines 286 to 287 in 9c418bc

#if __cplusplus >= 202002L || \
(__cplusplus >= 201709L && FMT_GCC_VERSION >= 1002)

fmt/test/compile-test.cc

Lines 190 to 191 in 9c418bc

#if __cplusplus >= 202002L || \
(__cplusplus >= 201709L && FMT_GCC_VERSION >= 1002)

So I just switched the Ubuntu image to ubuntu-20.04 for the g++-10 compiler with C++20 standard. The CompileTimeFormattingTest tests do run now, it's checked by failing CI build with false commit on CI.

Also, this PR fixes the problem that was not detected on CI because of the wrong GCC-10 minor version.

@alexezeder alexezeder marked this pull request as draft January 23, 2021 13:23
the problem was not detected by test because of wrong gcc-10 minor version on CI
@alexezeder alexezeder force-pushed the fix/wrong_gcc_10_minor_version_on_CI branch from 7568ddc to 92835d3 Compare January 23, 2021 13:32
@alexezeder alexezeder marked this pull request as ready for review January 23, 2021 13:46
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one comment inline.

include:
- cxx: g++-4.8
install: sudo apt install g++-4.8
os: ubuntu-18.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid explicitly specifying ubuntu-18.04 here and below? Won't it get the default from os above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My tests with Actions show that no, it won't be defaulted to ubuntu-18.04, it's just empty in those cases.
Here is the CI run for config without explicit os.

@vitaut vitaut merged commit acef0bb into fmtlib:master Jan 23, 2021
@vitaut
Copy link
Contributor

vitaut commented Jan 23, 2021

Merged, thanks!

@alexezeder alexezeder deleted the fix/wrong_gcc_10_minor_version_on_CI branch January 23, 2021 15:53
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.

2 participants