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.

Sign up for GitHub

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

Jump to bottom

cmake: fix fallthrough warnings with ccache #12827

Closed
wants to merge 1 commit into from
Closed

cmake: fix fallthrough warnings with ccache #12827

wants to merge 1 commit into from

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Aug 28, 2019 •
edited
Loading

It turns out that we're getting compile warnings with arm-none-eabi-gcc 7.2.1 when used with ccache about implicit fallthroughs. By setting CCACHE_CPP2 we force ccache to look at the whole file not just the stripped version.

Fixes #12813, closes #12820.

@julianoes julianoes requested review from dagar and baumanta August 28, 2019 13:57
@julianoes julianoes mentioned this pull request Aug 28, 2019
Merged
dagar
dagar reviewed Aug 28, 2019
View reviewed changes
CMakeLists.txt Outdated

# To prevent "// fallthrough" comments from being stripped by the preprocessor
# before ccache looks at the cache.
set(CCACHE_CPP2 "yes")
Copy link
Member

Choose a reason for hiding this comment

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

This would have to be set as an environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but it seems to work 🤷‍♂️ ...

Copy link
Contributor

Choose a reason for hiding this comment

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

If this goes in, just let me know what needa to change in docs and env setup scripts - if anything:-)

@baumanta
Copy link
Contributor

@julianoes I tried running this on my machine andt does not seem to help. If I build px4_fmu-v4_default, I still get the errors about implicit fallthrough

@julianoes
Copy link
Contributor Author

@julianoes I tried running this on my machine andt does not seem to help. If I build px4_fmu-v4_default, I still get the errors about implicit fallthrough

Maybe @dagar was right and I wasn't testing this correctly, let me check again.

@julianoes
Makefile: fix fallthrough warnings with ccache
b6de5ed 
It turns out that we're getting compile warnings with
arm-none-eabi-gcc 7.2.1 when used with ccache about implicit
fallthroughs. By setting CCACHE_CPP2 we force ccache to look at the
whole file not just the stripped version.
@julianoes
Copy link
Contributor Author

@baumanta check again please.

@baumanta
Copy link
Contributor

@julianoes
Now it is fine. I don't get errors anymore

@weekly-digest weekly-digest bot mentioned this pull request Sep 1, 2019
Closed
@julianoes
Copy link
Contributor Author

@dagar let's get this in.

@dagar
Copy link
Member

dagar commented Sep 2, 2019

A couple minor concerns.

  1. This only works if entering through our Makefile. Anyone using an IDE with proper integration or standalone cmake will bypass it.
  2. CCACHE_CPP2 is slower.
  3. Why aren't we seeing this in CI where we have nearly everything running with ccache and a couple builds explicitly with ccache disabled.

@julianoes
Copy link
Contributor Author

  1. True. However, I couldn't do it in cmake only. It didn't work.
  2. Right, that's not optimal.
  3. Yes, why not!? I want to understand.

@MaEtUgR
Copy link
Member

MaEtUgR commented Sep 3, 2019

Why aren't we seeing this in CI where we have nearly everything running with ccache and a couple builds explicitly with ccache disabled.

#12820 (comment)

@baumanta
Copy link
Contributor

baumanta commented Sep 3, 2019

@MaEtUgR @nicovanduijn maybe it makes sense to continue the discussion here, as this proposes a more reasonable solution.

I did a bisect and the warning appears for all commits after: [d41f72f] Re-enable implicit-fallthrough warning

@julianoes
Copy link
Contributor Author

I did a bisect and the warning appears for all commits after: [d41f72f] Re-enable implicit-fallthrough warning

Aha! Still odd why it wasn't caught by CI.

@jkflying
Copy link
Contributor

jkflying commented Sep 4, 2019

Downloaded and installed newest ccache from https://launchpad.net/ubuntu/bionic/amd64/ccache/3.4.1-1

This fixed the issue for me on 16.04.

@julianoes
Copy link
Contributor Author

@jkflying let's add that to the ubuntu.sh script as well as the scripts in the Devguide. (I know it's annoying that we currently have two scripts).

@MaEtUgR
Copy link
Member

MaEtUgR commented Sep 4, 2019 •
edited
Loading

@jkflying Confirmed:

wget http://launchpadlibrarian.net/356662933/ccache_3.4.1-1_amd64.deb
dpkg -i ccache_3.4.1-1_amd64.deb

fixes the problem instantly even without cleaning the cache.

cmake check for new ccache version with a message telling the user how to do so?

This was referenced Sep 5, 2019
Closed
Merged
@weekly-digest weekly-digest bot mentioned this pull request Sep 8, 2019
Closed
@julianoes julianoes closed this Sep 25, 2019
@julianoes julianoes deleted the fix-fallthrough branch September 25, 2019 08:22
This was referenced Oct 7, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamishwillee hamishwillee hamishwillee left review comments

@dagar dagar dagar left review comments

@TSC21 TSC21 TSC21 approved these changes

@baumanta baumanta Awaiting requested review from baumanta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ubuntu 16.04 ccache 3.2.4 Fallthrough Bug
7 participants
@julianoes @baumanta @dagar @MaEtUgR @jkflying @TSC21 @hamishwillee