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

Some warnings counts are not reliable #53

Closed
dreamer opened this issue Nov 24, 2019 · 5 comments · Fixed by #55
Closed

Some warnings counts are not reliable #53

dreamer opened this issue Nov 24, 2019 · 5 comments · Fixed by #55
Assignees
Labels
bug Something isn't working

Comments

@dreamer
Copy link
Member

dreamer commented Nov 24, 2019

Once we started counting compiler warnings in all the jobs and all compilers, we seem to be immediately hit by warnings numbers appearing inconsistent for some configurations.

At least one of causes behind this is: logs from several compilation jobs are being merged unreliably (not line-by-line), while count-warnings script depends on a warning being reported in a single line e.g.:

https://github.com/dreamer/dosbox-staging/runs/318215850#step:6:20 (227 warninigs)
https://github.com/dreamer/dosbox-staging/runs/318228442#step:6:20 (210 warnings)

The difference is in format (-9), deprecated-declarations (-7), and deprecated-register (-1). I started investigation with deprecated-register, and in the actual number of warnings is the same in both logs (9), but script does not count one occurence because of this:

https://github.com/dreamer/dosbox-staging/runs/318228442#step:5:786

It should look like this:

./render_templates_sai.h:188:4: warning: 'register' storage class specifier is deprecated and incompatible with C++17 [-Wdeprecated-register]
                        register int r = 0;
                        ^~~~~~~~~

but looks like this:

./render_templates_sai.h:188:4: warning: 1 warning generated.
'register' storage class specifier is deprecated and incompatible with C++17 [-Wdeprecated-register]
                        register int r = 0;
                        ^~~~~~~~~

(not sure why Clang decided to issue this warning for code compiled with C++11, but that's besides the point)

Text 1 warning generated/X warnings generated seems to be issued only on Clang.

What is the exact root cause? I'm not sure, candidates being:

  • maybe we shouldn't merge stdout with stderr to compile build.log (but it seems to work ok for GCC)?
  • maybe Clang some issues with buffering output? (I find it unlikely)
  • maybe colorized output contributes to the problem (e.g. by triggering binary output in Clang or something)

Running compilation with -j1 is not acceptable in my opinion, and we avoided it so far only because top-warnings job happened to be GCC. The issue is not macOS specific, it happened to Clang on Linux as well.

@krcroft any ideas how to resolve this and keep th clang warnings count usable?

@dreamer dreamer self-assigned this Nov 24, 2019
@dreamer dreamer added the bug Something isn't working label Nov 24, 2019
@dreamer
Copy link
Member Author

dreamer commented Nov 24, 2019

@kcgen
Copy link
Member

kcgen commented Nov 24, 2019

Fascinating and very subtle!

Common to have interspersed output with uncontrolled parallelized processes (just using the shell); but programs like gnu parallel know how to capture serialize output and presumably make does as well, given GCC is (or seems) unaffected.

Is Clang or make doing something wrong or harmful toward the other? Possibly an issue for either of these packages upstream too. Although I can see Apple saying "wont-fix, use json output").

Curious what you discover!

@kcgen
Copy link
Member

kcgen commented Nov 24, 2019

https://www.gnu.org/software/make/manual/html_node/Parallel-Output.html

Could try adding that to make in the os-(darwin, linux, and msys2) configs in https://github.com/dreamer/dosbox-staging/tree/master/scripts/automator/build

@dreamer
Copy link
Member Author

dreamer commented Nov 24, 2019

Yup, it seems to be the solution --output-sync=line solved the issue for Clang on Linux just like that. On macOS there are caveats (of course) - make provided by Xcode (used for both our jobs) does not support this option. Switching to brew-provided gmake solves it.

I have proof of concept ready - I'll turn it into proper PR later; keeping the issue open until then.

@kcgen
Copy link
Member

kcgen commented Nov 24, 2019

Nice! (And sadly.. quite a trend for macOS).

dreamer added a commit that referenced this issue Nov 26, 2019
This works out of the box on Linux and MSYS2, but does not work on
macOS - Xcode supplied make does not support this option, so GNU make is
used instead.

Unfortunately, adding new package on macOS did not invalidate the cache,
this package removes the brew cache from macOS job to avoid this problem
in the future.

Fixes: #53
dreamer added a commit that referenced this issue Nov 26, 2019
This works out of the box on Linux and MSYS2, but does not work on
macOS - Xcode supplied make does not support this option, so GNU make is
used instead.

Unfortunately, adding new package on macOS did not invalidate the cache,
this package removes the brew cache from macOS job to avoid this problem
in the future.

Fixes: #53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants