Fix CMAKE_ARGS that may have spaces inside quotes

[vyasr]

The current logic for parsing CMAKE_ARGS assumes that it is safe to split the string on spaces. However, this may not be the case for all variables; some variables must be passed with spaces in them, but inside quotes. The CMAKE_<LANG>_FLAGS are a prime example of this. Quoting from the docs:

This value is a command-line string fragment. Therefore, multiple options should be separated by spaces, and options with spaces should be quoted.

To support this, the split needs to be done with a regular expression instead of a simple string split.

I left a note in the test regarding the fact that this behavior is sensitive to quotes. I don't know if we can or should do anything about that, though.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.97%. Comparing base (60d5152) to head (59bff0c).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #727      +/-   ##
==========================================
+ Coverage   81.88%   81.97%   +0.08%     
==========================================
  Files          68       68              
  Lines        3886     3888       +2     
==========================================
+ Hits         3182     3187       +5     
+ Misses        704      701       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[henryiii]

What about using shlex.split?

[henryiii]

Also, a unit test might be a lot cheaper than an integration test just for this function. Edit: I moved it to a C-based test, it only takes 2 seconds, so I think that's fine.

[henryiii]

Thoughts on this version with she’s.split?

[vyasr]

Wow that was quick. Sorry I didn't get a chance to respond in time. My thoughts:

• The shlex.split version is definitely better, it just didn't occur to me at the time since I rarely use that module and I reached for a regex by default.
• I'm glad you were able to get a faster compile by using C instead of C++. I do think an integration test is better than just a unit test here because there are multiple levels of translation happening from the environment variable to what your source code actually sees, and I probably wouldn't trust a unit test to catch all cases. For example, the issue with the difference between double and single quotes only shows up if you actually get to the point of executing a compile line (of course, in this case actually doing anything with that information is probably out of scope, but I wouldn't have necessarily predicted that a priori).