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

Feature/package id cmake checks #6659

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Mar 11, 2020

Changelog: Bugfix: Use the real settings value to check the compiler and compiler version in the cmake generator local flow when the package_id() method changes values.
Docs: Omit

Fix: #6658

jgsogo
jgsogo approved these changes Mar 11, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Probably this change makes no difference as we are unsing only CMAKE_MATCH_1 , but at least it matches the comment and the previous block of code... although MATCH will probably run faster.

Copy link
Member

@jgsogo jgsogo left a comment

Wow, sorry, I was looking to one commit of the changeset (not my best day).

I don't see how this change fixes the issue, the regex continues to be the same, does the CMAKE_MATCH_1 stores the last match when using MATCHALL?

jgsogo
jgsogo approved these changes Mar 11, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Ok, according to CMake docs: https://cmake.org/cmake/help/v3.17/variable/CMAKE_MATCH_n.html

CMAKE_MATCH_: Capture group <n> matched by the last regular expression

@memsharded
Copy link
Member Author

@memsharded memsharded commented Mar 11, 2020

Changed, so instead of using MATCHALL, looks first for full_settings. Please re-review.

@memsharded memsharded requested a review from jgsogo Mar 11, 2020
Copy link
Member

@jgsogo jgsogo left a comment

If we are going to match the full_settings, why not using a regex that will match only that block like \[full_settings((?!\[full_).)*?

And really important now, how does CMake's regex engine work with new lines? With this approach the .* pattern needs to match also new lines (usually it needs to activate a flag s), but I see that tests are passing so it should be the default. Do we trust CMake about this?

@memsharded
Copy link
Member Author

@memsharded memsharded commented Mar 11, 2020

Yes, that is the problem. Trying to write such a regex is pretty ugly, like needing \\\\[full_settings\\\\] and still the problem of new lines. So I preferred to do the search for full_settings, and just capture the remaining of the file, that should work for this issue, and in any case is something that will start to change soon.

jgsogo
jgsogo approved these changes Mar 11, 2020
@memsharded memsharded merged commit f5596a8 into conan-io:develop Mar 11, 2020
2 checks passed
@memsharded memsharded deleted the feature/package_id_cmake_checks branch Mar 11, 2020
sthagen added a commit to sthagen/conan-io-conan that referenced this issue Mar 12, 2020
Feature/package id cmake checks (conan-io#6659)
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