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

Add C++ standards to the build matrix #1999

Closed
wants to merge 1 commit into from

Conversation

jgopel
Copy link
Contributor

@jgopel jgopel commented Nov 10, 2020

Problem:

  • A variety of compilers are tested in CI, but they are all tested on
    the C++11 standard. This prevents CI from catching bugs that exist
    only on a particular version of the C++ standard.

Solution:

  • Parameterize CI on C++ standards 11, 14, 17, and 20.

This also includes fixes for compiler warnings discovered along the way. I've included a detailed writeup of each compilation error and the fix in the commit notes for each fix.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

@jgopel
Copy link
Contributor Author

jgopel commented Nov 10, 2020

I wrote a little script that generates a build directory for every possible compiler version & standard version combination. It's nothing fancy, but getting the details right took a few minutes. Would there be any interest in upstreaming that? Maybe something like support/generate-all-builds.py?

@jgopel
Copy link
Contributor Author

jgopel commented Nov 10, 2020

Looks like this partially duplicates #1992 - not sure how to proceed here.

@alexezeder
Copy link
Contributor

alexezeder commented Nov 10, 2020

Looks like this partially duplicates #1992 - not sure how to proceed here.

Closed. You may want to read my final words there.

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.

Thanks for the PR. Looks good in general, just a few minor comments.

@vitaut
Copy link
Contributor

vitaut commented Nov 12, 2020

Would there be any interest in upstreaming that? Maybe something like support/generate-all-builds.py?

Probably not but you can post it here in case anyone is interested.

@jgopel
Copy link
Contributor Author

jgopel commented Nov 12, 2020

Would there be any interest in upstreaming that? Maybe something like support/generate-all-builds.py?

Probably not but you can post it here in case anyone is interested.

Here's a gist https://gist.github.com/jgopel/1d9fcdaf1f82c6f0440487cba791ee68

Problem:
- A variety of compilers are tested in CI, but they are all tested on
  the C++11 standard. This prevents CI from catching bugs that exist
  only on a particular version of the C++ standard.

Solution:
- Parameterize CI on C++ standards 11, 14, 17, and 20.
@jgopel jgopel force-pushed the support-additional-standards branch from fefe33e to 1f6635e Compare November 13, 2020 13:04
@vitaut
Copy link
Contributor

vitaut commented Nov 14, 2020

Closing since the new standards have been added in f8640d4 but thanks for the PR.

@vitaut vitaut closed this Nov 14, 2020
@jgopel
Copy link
Contributor Author

jgopel commented Nov 14, 2020

Closing since the new standards have been added in f8640d4 but thanks for the PR.

I'm not sure that does what you want it to do, @vitaut. I had experimented with that approach and it didn't generated both debug and release jobs for those standards. See: https://github.com/jgopel/fmt/actions/runs/361646531 and https://github.com/jgopel/fmt/actions/runs/361658036. The only way I've found success so far is with an approach like this https://github.com/jgopel/fmt/actions/runs/363543436.

Note that there's an open issue with GitHub Actions about roughly this problem. You can find that here: https://github.community/t/reusing-sharing-inheriting-steps-between-jobs-declarations/16851

@jgopel
Copy link
Contributor Author

jgopel commented Nov 14, 2020

I've updated this to reflect my latest thinking about what this feature should look like - including support for using an older OS as discussed by @alexezeder and @darklukee on #1992. No pressure to merge this, just wanted to document what I've learned and put forth my best attempt at a solution that has all the desired attributes.

Edit: It seems that PRs stop updating once they're closed, so at time of writing the SHAs of interest for my final proposed changeset are cddc7ea and cc60505.

@vitaut
Copy link
Contributor

vitaut commented Nov 14, 2020

it didn't generated both debug and release jobs for those standards

That's fine.

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.

3 participants