I don't think the doCheck switch is even necessary. If someone will want to disable tests they'll always be able to use overrideAttrs.

doCheck is necessary so that I can tell cmake to not build the tests if it's false. Otherwise, the build would fail because dependencies from checkInputs would be ignored in this situation.

To ensure consistent behavior while allowing the builds to succeed, I have changed doCheck to respect the value of config.doCheckByDefault.

From my experience, checkInputs don't even enter the environment if doCheck is not true.

Yes, that's why I kept doCheck. https://github.com/NixOS/nixpkgs/pull/89223/files#diff-b6364e583825dc39777b37eb0c055a50R22

https://github.com/NixOS/nixpkgs/pull/89223/files#diff-b6364e583825dc39777b37eb0c055a50R22

I saw that commit. I'm just not sure I totally understand this:

doCheck is necessary so that I can tell cmake to not build the tests if it's false.

I claim, that with doCheck set to true unconditionally, and someone wishing to set it to false with an overrideAttrs, the tests won't even be built because gtest is in checkInputs (vs nativeBuildInputs). Meaning, that it's not needed to put this switch in the top level arguments - for all purposes, overrideAttrs setting doCheck to false will be enough.

I wasn't aware of config.doCheckByDefault until today. If there is a configuration, I think it's best to respect it.

With all due respect to user configs, my personal opinion is that checks should be enabled by default where they don't fail - it helps detecting issues in updates by @r-ryantm where one may feel lazy to try to catch any by actually using the package they updated, especially if it's a library. I think also that committers like checks to be run if they are available and they don't fail. That's why it is enabled by default in the Python's ecosystem and they've even went as far as requesting contributors to add import tests if no tests are available by upstream ( pythonImportsCheck).

I once added a superficial check for a package I maintain at: https://github.com/NixOS/nixpkgs/pull/87734/files so that I'd be able to spare my self the need to check it myself upon updates.

But anyway, that's your choice. Let's hope your PR will even get any attention as it seems like hard times lately with the amount of time committers have to put into Nixpkgs :).

I'm not really sure about the purpose of config.doCheckByDefault, so I'm fine with ignoring it if it's not meant for strict enforcement. But I'd like to hear from committers before I actually do anything further.

If there's not a specific reason not to, derivations should always run unit tests, as it facilitates spotting errors during version bumps. When someone wants to bump the package, and tests suddenly fail, most likely getting the tests back to work requires editing the derivation anyhow, so we should just set doCheck = true; here.

Thanks, I have now set doCheck = true back again.

On the other hand, I'd put gtest in checkInputs.

Done

I'm up to keeping this line on the other hand.