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 W=1 to Makefile #64

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

aleksamagicka
Copy link
Member

@aleksamagicka aleksamagicka commented Dec 5, 2023

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
Copy link
Member

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
Copy link
Member Author

aleksamagicka commented Dec 5, 2023

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
Copy link
Member

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
Copy link
Member Author

aleksamagicka commented Dec 5, 2023

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
Copy link
Member Author

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 aleksamagicka marked this pull request as ready for review December 7, 2023 18:42
@aleksamagicka
Copy link
Member Author

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
Copy link
Member

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

@aleksamagicka
Copy link
Member Author

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

@jonasmalacofilho jonasmalacofilho merged commit 85fa359 into liquidctl:master Dec 12, 2023
11 checks passed
@aleksamagicka aleksamagicka deleted the makefile-w1-c1 branch December 16, 2023 10:41
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.

2 participants