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 Warnings Next Generation Plugin #359

Closed
wants to merge 9 commits into from

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Dec 14, 2019

The Warnings plugin currently in use is deprecated. This adds the warnings-ng plugin to replace it.
See also #358 which removes the old plugin.

@dirk-thomas
Copy link
Member

Adding @nuclearsandwich as a reviewer since he is in charge of our Jenkins infrastructure.

@nuclearsandwich
Copy link
Member

@rotu are you testing this PR against an internal Jenkins deployment? I would like to set it up on my test CI deployment but knowing what state to expect would really help there.

@rotu
Copy link
Contributor Author

rotu commented Dec 22, 2019

I don’t know how to test this PR. I cobbled it together using Jenkins installed on my Mac.

@rotu
Copy link
Contributor Author

rotu commented Feb 18, 2020

@nuclearsandwich rebased.

@j-rivero j-rivero assigned j-rivero and unassigned nuclearsandwich Feb 27, 2020
@j-rivero
Copy link
Contributor

@nuclearsandwich rebased.

Thanks Dan. I've looked into the changes, mostly looks good to me. I'm afraid of unconditionally enable all compiler parsers in all the jobs, not sure if the new plugin handle it in a smart way. Just in case, I've add some code to only enable compilers based on platforms or the use of clang. Feel free to comment or cherry-pick.

I'm planning to give this changes and the update of the plugin a run on a testing build farm.

@rotu
Copy link
Contributor Author

rotu commented Feb 27, 2020

@j-rivero I don't love that change - we should be basing it on (1) the identity of the actual compiler (2) just the output of that compiler, not the output of other things in the log like tests. Still I suppose it's better than no filtering.

@j-rivero
Copy link
Contributor

@j-rivero I don't love that change - we should be basing it on (1) the identity of the actual compiler

Do we have a better way of detecting the actual compiler at the moment of generating the job configuration files? I agree that the approach is not rock-solid but given the limited number of options I think that is reasonable at this point to use compile_with_clang_default and assume osx = clang and windows = msbuild.

(2) just the output of that compiler, not the output of other things in the log like tests. Still I suppose it's better than no filtering.

How changes in this PR can limit the scope in which the warning parser operates?

@rotu
Copy link
Contributor Author

rotu commented Feb 27, 2020

How changes in this PR can limit the scope in which the warning parser operates?

out of scope for this PR. Just a consideration for the future

@j-rivero
Copy link
Contributor

I agree that the approach is not rock-solid but given the limited number of options I think that is reasonable at this point to use compile_with_clang_default and assume osx = clang and windows = msbuild.

After given another look at this, there is a parameter at the moment of launching the build that allows the user to select Clang in Linux instead of Gcc. I've changed the code to run all the times gcc+clang on Linux, clang on Osx and msbuild on Win. Parsers have been typically fast even in long logs, I would not expect significant increases of time even with various parsers enabled.

@rotu would you mind pulling changes from master...j-rivero:warning_new_gen? (for some reason I can not push to your branch sorry). Let me know if you prefer that I open a different PR.

@rotu
Copy link
Contributor Author

rotu commented Feb 28, 2020

@j-rivero Rebased and merged your branch in.

@j-rivero j-rivero requested a review from dirk-thomas February 29, 2020 00:01
@j-rivero
Copy link
Contributor

merge ready in #404

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.

4 participants