liquidctl / liquidtux

Linux kernel hwmon drivers for AIO liquid coolers and other devices
Other
79 stars 14 forks source link

Add W=1 to Makefile #64

Closed aleksamagicka closed 9 months ago

aleksamagicka commented 9 months ago

Related to #60. Add W=1 to Makefile.


Marking as a draft because I'd like to also add C=1, but I had to install sparse myself, so I need to investigate the Github CI.

jonasmalacofilho commented 9 months ago

Just one thought: should we add W=1 C=1 to our Makefile our just to our CI jobs?

The downside I see in forcing the checks in the Makefile is that it'll affect all users of the yet-out-of-tree drivers. Enabling W=1 is probably not a big deal, but having to install sparse just to build and use the drivers seems inconvenient.

aleksamagicka commented 9 months ago

Yeah, I think that's the way to go. Users should be able to compile and insert regardless of us missing an extended declaration or something.

jonasmalacofilho commented 9 months ago

Actually, to clarify, on W=1 alone, I personally prefer to have it always (or at least by default) enabled for me.

But, still, that's not the default in C or kernel land...

aleksamagicka commented 9 months ago

Actually, to clarify, on W=1 alone, I personally prefer to have it always (or at least by default) enabled for me.

Me too, but...

But, still, that's not the default in C or kernel land...

That's the thing, I've seen drivers in hwmon that fail with W=1 with today's tree. Arguably, if W=1 is in CI, then the code here should pass it, unless the definition of it changes?

C=1 should be left to CI, I think.

Sorry for the jumbled comment, I'll think more about it, but I'd definitely like to have W=1 in CI at least.

aleksamagicka commented 9 months ago

Yeah, I think that's the way to go. Users should be able to compile and insert regardless of us missing an extended declaration or something.

Whoops, should have checked first before making this assumption. Here W=1 just gives a warning, while in the kernel it stops the compilation. In that case, as you said, I think it's OK to turn it on for everyone (as it's useful to have during development), but CI would need to treat the warnings it gives as errors, so there's still a distinction. I'll look into that.

aleksamagicka commented 9 months ago

Here's how this looks like:

sparse seems to generate a lot of warnings that have nothing to do with the drivers in here, so it looks like I'll have to extend the problem matcher to catch messages related to code from this repo.

jonasmalacofilho commented 9 months ago

If I'm not mistaken, this can be merged as is. Or do you prefer to first improve the problem matcher?

aleksamagicka commented 9 months ago

Let's merge it as is and I'll deal with the problem matcher later (I'm somewhat short on time right now).