In D4861#140658, @karol wrote:

Weird that some cmake rules don't allow upper case letters for variable names (which is pretty standard I'd say), the regex is: [a-z][a-z0-9_]+

@atul should we fix this?

I'm publishing this because I don't think this is an error.

I'm not sure if there is a good reason for this (CMake can have strange requirements...) or maybe it is just a "style guide" thing for CMake.

That said, does the variable type really matter? It doesn't seem like a great use of people's time to figure out a way to revise this...

I think @jon is more likely to know the answer here than @atul, but my general suggestion is just to use lowercase variable names to appease the CI...

In D4861#140927, @karol wrote:

Ok, once your diff is landed, I'm going to rebase to it and take your version removing mine, no problem. How about the rest of this diff?

Thanks, @karol ! I've added a few small notes.

That said, does the variable type really matter? It doesn't seem like a great use of people's time to figure out a way to revise this...

It's about names, not types. I think we should resolve this. The same checks seem to run on the CMakelist.txts and *.cmake files but still with the corrosion.cmake we get errors and with the rest of the files we do not even though there seem to be very similar definitions in both files.

• thie is error set(ONE_VALUE_KEYWORDS PATH NAMESPACE CXX_BRIDGE_SOURCE_FILE)
• this is not set(BUILD_TESTING OFF CACHE BOOL "Turn off tests" FORCE)

So I perceive this as an invalid CI action (false positive).

I think @jon is more likely to know the answer here than @atul, but my general suggestion is just to use lowercase variable names to appease the CI...

That doesn't solve anything. I think we should either turn off this rule or get it fixed.

Sorry, I meant variable name not type. If the variable name is your decision then I think we can just follow CMake Lint's requirements. If the variable name is decided by somebody else, then we can disable the rule like we do here.

Curious for @jon's perspective on this one too.

Sorry, but it seems to me like you didn't really address what I said:

The same checks seem to run on the CMakelist.txts and *.cmake files but still with the corrosion.cmake we get errors and with the rest of the files we do not even though there seem to be very similar definitions in both files.

• this is error set(ONE_VALUE_KEYWORDS PATH NAMESPACE CXX_BRIDGE_SOURCE_FILE)
• this is not set(BUILD_TESTING OFF CACHE BOOL "Turn off tests" FORCE)

So I perceive this as an invalid CI action (false positive).

How about this?

I do not think that CI should behave like this, it's not logical to me, and would be cool to find out what exactly is going on here.

I will disable the rule but I think it's more like a dirty workaround than a reasonable solution.

@max you've been blocking this for almost a week now, could you please do a review?

I think @jon is more likely to know the answer here than @atul, but my general suggestion is just to use lowercase variable names to appease the CI...

There's no rhyme or reason with CMake naming conventions. Every ecosystem seems to have created their own convention over CMake's history. It's just that cmake-lint establishes some conventions which make it unambiguous about variable usage.

Why it errors in some usages, but not other usages; I have no idea.

In D4861#143177, @ashoat wrote:

Sorry @karol but I would prefer to wait until D4805 lands (hopefully tomorrow), then remove all of the CMake from here before landing. Given past experience I suspect we will end up forking things... there will be some Backlog task to merge the CMakes and it will keep getting punted...

D4805 is landed now.