Make Jenkins check if a PR will add new warnings #768
Conversation
Signed-off-by: Angie <angheloalf95@gmail.com>
Signed-off-by: Angie <angheloalf95@gmail.com>
|
Is this really necessary now that we're almost at the point where we can elevate warnings as errors to prevent them entirely? |
|
We have many warnings that will stay in the repo because they are errors made by the original devs. For example, see this warning. The idea is prevent new warnings being merged without us noticing. Every PR should be as clean as possible. |
|
We can always prevent or disable the few warnings that can't be removed entirely, and I don't think these warnings would be worth keeping over the benefits of treating warnings as errors. Especially considering that if they come from a potential bug or UB, the code will be annotated as such anyway (and in the case of UB it will even be avoided entirely with a flag like AVOID_UB in the future). I might be missing some possible issues, but I looked at most of the warnings we have left the other day, and at this point I think we could do it without too much trouble if we wanted to. |
|
Either way I agree about trying to prevent new warnings from being introduced, so I'm not necessarily against this solution. It could very well be a temporary solution like you said. But yeah imo we should consider dealing with warnings completely to keep things simple for everyone. |
|
The other benefit of this is it will check warnings emitted by in other compilation stages, like warnings produced by ZAPD. |
|
Is there any interest in adding this to the repo? |
|
yea sorry, just been prioritizing other prs. thanks for the reminder I personally feel like it might be too early to be enforcing this, but I am not 100% sure on that. If this is something a majority of people would want to see now, then I wouldnt be against it. Just want to make sure its something everyone thinks is good to introduce now. Havent heard from many contributors other than roman above |
|
Early? The project is over 75% I think we should try to aim to have cleaner code. And if it is not possible because of some dummy zelda dev (like some weird cases we have in the repo), there is a script that allows updating the current amount of warnings, so Jenkins wont complain at the PR. Would be nice if more people gave their opinions too :D |
|
Early as in, we are still in the decomp phase and that is our main priority. To me, that kind of cleanup is something secondary that should be focused on after the initial decomp is completed. |
|
I agree that I don't think it is the right time to do stuff like this, and its worth noting that nintendo's code was more than likely not warning free, and some warnings are just gonna have to be there |
We currently enforce this in MM. I think that it's a good code quality indicator. There is a clear red flag for when new warnings are added and a clear process to add new ones that are required. |
|
I'd almost argue that it's too late now, but I would say that... I think provided it's explained in documentation what to do if your PR fails because of new warnings, it's fine. It looks like a good implementation to avoid accidentally introducing new warnings, and there's a simple way to inform it that new warnings are required. Indeed, with this implementation, it should be easier to find out what the new warnings are anyway: you can just look at the generated file (or even the file difference). |
|
Yea I mean, I think you have that opinion cause you took on alot of the manual work to fix them. Which is very appreciated. But I dont think fixing them now vs when the percent counter is at 100% would have made all too much of a difference. The codebase is going to have a finetooth comb run through it many times. And for me, getting that decomp percentage up is the #1 priority before anything else |
|
But yea, like I mentioned originally Im not super against it. Just want to make sure a majority of the contributors are on board as well FeelsOkMan |
|
and yea its worth noting @Zelllll
the way its set up, if we are confident that a warning will have to exist with no reasonable fix, it will be easy to allow an exception for it. This just forces us to make the consious decision right away that its a warning that should stay. |
|
Yeah, exactly. Given that reviewers don't normally check that, I think it's good to get Jenkins to check it. Most of the time it's a 5 minute job to fix, and when it isn't it's worth knowing about sooner rather than later imo. (And yeah, of course the time I spent fixing a few hundred warnings has a bearing on how I feel about this!) |
|
As long as the process to allow a warning to slip in without compromising the integrity of the OK is not too overbearing, I think it'll be fine to have. The vast majority of the warnings I've seen through decomp are easily fixable stuff, and if we do end up in a situation where we end up with a bunch of warnings we can always just ignore it and pull, and fix them later |
I actually see this as a good thing: the more eyes on a warning when it initially appears, the more likely it is to be resolved, one way or the other, sooner. And further, from experience we all know that people have more motivation to fix warnings when there's not a whole cascade of them. Finally, as you noted earlier, there will be later passes to examine the warnings that we can't/don't want to fix now. |
|
Yea thats fair. Im on board if we have a general consensus |
|
this already has two approvals so probably not much use, but honestly after dealing with this in MM already the warning check thing is very frustrating to work with. Every single new warning that I supposedly added was the fault of someone else's file, and my pr's constantly failed Jenkins as a result. I don't see what the issue is in just fixing these warnings in bulk later on since we are so close to the end anyways, and it would be very easy to fix them all at once rather than making it incredibly frustrating for people to contribute. All this has done as far as I can tell is make contributing pointlessly more difficult for people. Just my two cents. |
|
Its a fair thing to mention. |
|
Well one example I had is that I noticed a function in MM didn't have a prototype at all. Any time this function was used there should have been a warning already, but yet when I gave it a prototype like it should've had to begin with my pr would then not build because of stuff that was never my fault to begin with since now the warnings of the wrongly typed argument being given to the function were considered new warnings, and I then had to search through the repo and fix every instance of it when it was never my fault to begin with. Then it also randomly claimed that a new warning that had nothing to do with anything that I touched was my fault, which didn't make any sense. |
Adds two new stages to Jenkins. The first one is used to check warnings produced by "make setup", while the second one checks the warnings produced by
make. This way we will always know if the amount of warnings increment.It is done by redirecting
stderrof each command to a file, and then compare the lines amount to the current line count (stored in a file intools/warning_count/).If by any reason we need to increment the accepted amount of warnings in the repo, then we can just edit the corresponding warnings file in
tools/warning_count/. The scripttools/warnings_count/update_current_warnings.shcan do this automatically.To test the amount of warnings locally, you can use the script
tools/warnings_count/create_new_warnings.shto create the new warnings files and compare them with thetools/warnings_count/compare_warnings.pypython script.