Fix #14334: Support more msbuild conditional constructs

[autoantwort]

No description provided.

[firewave]

Thanks for your contribution.

Please avoid adding test-only code in the production code. See e04019c for an approach. If you inherit from ProjectConfiguration the added function could be moved into the test code and the additional constructor and moved implementation could me made protected.

[firewave]

We should have filed a ticket for this since fixing it is a behavior change. Are you able to do that or should I file one?

[autoantwort]

Could you do that? :)

[firewave]

Will do later. I want to have a reproducer first.

[autoantwort]

Please avoid adding test-only code in the production code. See e04019c for an approach. If you inherit from ProjectConfiguration the added function could be moved into the test code and the additional constructor and moved implementation could me made protected.

Currently ProjectConfiguration and ConditionalGroup are an implementation detail and defined in the cpp file. I would have to move the classes to the header. Then they become an public interface only because of test-only code.
My thought was that exposing a single test-only function is better than exposing two complete implementation details classes.

[firewave]

Currently ProjectConfiguration and ConditionalGroup are an implementation detail and defined in the cpp file.

Sorry that I missed that.

I would have to move the classes to the header. Then they become an public interface only because of test-only code.
My thought was that exposing a single test-only function is better than exposing two complete implementation details classes.

Let's get the CI to pass first and the other stuff addressed and then I will have another look. It won't be merged before we made the 2.19 release anyways.

[autoantwort]

The selfcheck failure seems like an cppcheck error:
With 4f9f852 I get the error

Run supprs="--suppress=unusedFunction:lib/errorlogger.h:196"
lib/importproject.cpp:1676:21: style: The function 'selectVsConfigurations' is never used. [unusedFunction]
void ImportProject::selectVsConfigurations(Platform::Type platform, const std::vector<std::string> &configurations)
^
lib/importproject.cpp:1700:39: style: The function 'getVSConfigs' is never used. [unusedFunction]
std::list<std::string> ImportProject::getVSConfigs()
^

without I get

Run ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__CPPCHECK__ -D__GNUC__ -DQT_VERSION=0x060000 -DQ_MOC_OUTPUT_REVISION=69 -DQT_CHARTS_LIB -DQT_MOC_HAS_STRINGDATA --enable=unusedFunction,information --exception-handling -rp=. --project=cmake.output.notest/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr
lib/importproject.cpp:1677:0: information: Unmatched suppression: unusedFunction [unmatchedSuppression]
void ImportProject::selectVsConfigurations(Platform::Type platform, const std::vector<std::string> &configurations)
^
lib/importproject.cpp:1702:0: information: Unmatched suppression: unusedFunction [unmatchedSuppression]
std::list<std::string> ImportProject::getVSConfigs()
^

[autoantwort]

I only rebased with main

[danmar]

Why don't you tokenize as before? That would get rid of a lot of the string handling. And it is more flexible it will allow spaces between tokens. As far as I see your code requires for instance that there is no spaces in $(Configuration) .

[autoantwort]

Why don't you tokenize as before? That would get rid of a lot of the string handling. And it is more flexible it will allow spaces between tokens. As far as I see your code requires for instance that there is no spaces in $(Configuration) .

Because I only know that it didn't worked. Does the tokenizer know how to tokenize in strings? Feel free do create a PR at my feature branch :)

[danmar]

hmm.. I have tried to improve the existing functionality instead of rewriting it:
#8116
the exceptions are not matching anymore.
would it be possible for you to try to copy the needed changes and make it ready-to-merge?

[autoantwort]

I fixed the first for to

ASSERT_THROW_EQUALS_2(cppcheck::testing::evaluateVcxprojCondition("And", "", ""), std::runtime_error, "Invalid condition: 'And'");
ASSERT_THROW_EQUALS_2(cppcheck::testing::evaluateVcxprojCondition("Or", "", ""), std::runtime_error, "Invalid condition: 'Or'");
ASSERT_THROW_EQUALS_2(cppcheck::testing::evaluateVcxprojCondition("!", "", ""), std::runtime_error, "Invalid condition: '!'");
ASSERT_THROW_EQUALS_2(cppcheck::testing::evaluateVcxprojCondition("'' == '' And ", "", ""), std::runtime_error, "Missing operator");

but after that the program simply crashes with a read access violation.

[danmar]

Thanks! I have fixed that in my PR. I would say that my exception texts are not very good so please feel free to adjust and improve them.
I would like to understand how cppcheck output looks if there is an invalid condition. Does cppcheck say which file the invalid condition is in? I assume it does not say which line the condition is on? Not sure if tinyxml provides the line..

What the cppcheck output is.. that is something that should be tested in test/cli/.. If you are not good at pytest I can add a test for it but I would like that you create some minimal vcxproj file for me then that can be used in such test.

[danmar]

it's not clear to me directly why we get those unmatchSuppression warnings in selfcheck but looks like it more or less works!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[danmar]

I merged #8258 that I guess provided the same code. So I close this PR. Feel free to reopen and cleanup if there is something important in this PR.