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

Fix MacOS configuration regex. #3297

Merged
merged 3 commits into from Nov 4, 2022
Merged

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Oct 25, 2022

I found this issue while packaging mlpack for Julia. It seems like the ?: operator isn't supported by CMake regexes, so I adapted them a little bit accordingly. These were the errors being seen:

[15:09:34] CMake Error at CMakeLists.txt:187 (string):
[15:09:34]   string sub-command REGEX, mode REPLACE failed to compile regex
[15:09:34]   "([0-9]+)(?:\.([0-9]+).*)*".
[15:09:34] 
[15:09:34] 
[15:09:34] CMake Error at CMakeLists.txt:192 (string):
[15:09:34]   string sub-command REGEX, mode REPLACE failed to compile regex
[15:09:34]   "([0-9]+)(?:\.([0-9]+).*)*".

from this build log.

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Oct 25, 2022

I found this issue while packaging mlpack for Julia. It seems like the ?: operator isn't supported by CMake regexes, so I adapted them a little bit accordingly. These were the errors being seen:

[15:09:34] CMake Error at CMakeLists.txt:187 (string):
[15:09:34]   string sub-command REGEX, mode REPLACE failed to compile regex
[15:09:34]   "([0-9]+)(?:\.([0-9]+).*)*".
[15:09:34] 
[15:09:34] 
[15:09:34] CMake Error at CMakeLists.txt:192 (string):
[15:09:34]   string sub-command REGEX, mode REPLACE failed to compile regex
[15:09:34]   "([0-9]+)(?:\.([0-9]+).*)*".

from this build log.

I have used ?: for a non-capturing group. You can simply remove this and use capture number 3 instead of 2. It also maybe the CMake version issue cause here, all builds are working perfectly as of now.

CMakeLists.txt Outdated
"([0-9]+)(?:\\.([0-9]+).*)*" "\\2"
"([0-9]+)\\.([0-9]+)(\\.([0-9]+).*)*" "\\2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "([0-9]+)(\\.([0-9]+).*)*" "\\3" instead. Maybe it will help in version string like 10

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misunderstood the original regex---I thought the idea here was to capture the second (minor) version number. So wouldn't it suffice to split out the second ([0-9]+) capture group and use that, and leave the (\\.([0-9]+).*)* to capture (and leave unused) any additional terms in the version number?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the purpose of this regex is to get the second (minor) version number if it exists. Before this fix, we used forced major version 10 and then a minor number. But after that fix, we also took care of major version 11. Now, I don't remember whether it would not break if we didn't include cases with no minor number like "11".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation; fixed in 28be9de. I tested this and it seems to work fine. 👍

Copy link
Contributor

@shubham1206agra shubham1206agra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me!

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit 6f5379f into mlpack:master Nov 4, 2022
@rcurtin rcurtin deleted the fix-macos-regex branch November 4, 2022 15:02
@qazi0
Copy link

qazi0 commented Dec 22, 2022

@rcurtin Any plans to ship this to a patch (4.0.1?) release soon? As this is still broken in 4.0.0 on macos. Had to manually apply the patch from this commit to get it to work

@rcurtin
Copy link
Member Author

rcurtin commented Dec 22, 2022

@Siraj-Qazi sorry that you had to apply it manually. Let me merge #3322 (today or tomorrow), then I'll open a PR for the 4.0.1 release, which will take a day or two to merge. 👍

@qazi0
Copy link

qazi0 commented Dec 22, 2022

Thanks alot @rcurtin !

@rcurtin rcurtin mentioned this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants