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: MSBuildDeps generator: CAExcludePath #8682

Merged
merged 7 commits into from Mar 29, 2021

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Mar 22, 2021

closes: #8633

Changelog: Feature: MSBuildDeps generator: CAExcludePath
Docs: TODO

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

Signed-off-by: SSE4 <tomskside@gmail.com>
conan/tools/microsoft/msbuilddeps.py Outdated Show resolved Hide resolved
conan/tools/microsoft/msbuilddeps.py Outdated Show resolved Hide resolved
@@ -25,6 +26,7 @@ class MSBuildDeps(object):
<Conan{name}BinaryDirectories>{bin_dirs}</Conan{name}BinaryDirectories>
<Conan{name}Libraries>{libs}</Conan{name}Libraries>
<Conan{name}SystemDeps>{system_libs}</Conan{name}SystemDeps>
<Conan{name}CAExcludeDirectories>{ca_exclude_dirs}</Conan{name}CAExcludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

This is not new "data" definition, shouldn't be defined here, but just in the group below, as an if ca_exclude_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I follow - it uses the same structure as another definitions...

Copy link
Member

Choose a reason for hiding this comment

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

This section contains the pure translation of cpp_info fields, nothing more.
The custom logic of this generator is not defined in this section, but somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, then I agree it shouldn't be there

@@ -42,6 +44,7 @@ class MSBuildDeps(object):
<PropertyGroup>
<LocalDebuggerEnvironment>PATH=%PATH%;$(Conan{name}BinaryDirectories)$(LocalDebuggerEnvironment)</LocalDebuggerEnvironment>
<DebuggerFlavor>WindowsLocalDebugger</DebuggerFlavor>
<CAExcludePath>$(Conan{name}CAExcludeDirectories);$(CAExcludePath)</CAExcludePath>
Copy link
Member

Choose a reason for hiding this comment

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

Put the conditional here, to define it or not (by default, this line shouldn't appear)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do I need condition here? $(Conan{name}CAExcludeDirectories) will just resolve to an empty string, so line will have no effect.

conan/tools/microsoft/msbuilddeps.py Outdated Show resolved Hide resolved
SSE4 and others added 3 commits March 22, 2021 23:39
Co-authored-by: James <james@conan.io>
Co-authored-by: James <james@conan.io>
Co-authored-by: James <james@conan.io>
exclude_code_analysis = self._conanfile.conf["tools.microsoft.msbuilddeps"].exclude_code_analysis
if exclude_code_analysis is not None:
self.exclude_code_analysis = eval(exclude_code_analysis)
assert isinstance(self.exclude_code_analysis, (bool, list))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@memsharded I've applied your suggestions, but now, string pattern doesn't work any longer, as you accept only bool and list here.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I have pushed some changes

@@ -25,6 +26,7 @@ class MSBuildDeps(object):
<Conan{name}BinaryDirectories>{bin_dirs}</Conan{name}BinaryDirectories>
<Conan{name}Libraries>{libs}</Conan{name}Libraries>
<Conan{name}SystemDeps>{system_libs}</Conan{name}SystemDeps>
<Conan{name}CAExcludeDirectories>{ca_exclude_dirs}</Conan{name}CAExcludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

This section contains the pure translation of cpp_info fields, nothing more.
The custom logic of this generator is not defined in this section, but somewhere else.

ca_exclude = self._conanfile.conf["tools.microsoft.msbuilddeps"].exclude_code_analysis
if ca_exclude is not None:
# TODO: Accept single strings, not lists
self.exclude_code_analysis = eval(ca_exclude)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

are we really okay if eval usage? it seems to be pretty dangerous, as it may allow arbitrary code execution:
https://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html

Copy link
Member

Choose a reason for hiding this comment

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

Yes, as well as every Conan recipe, how this would be more dangerous than Conan python evaluation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can avoid eval then? if use just a regexp, if regexp matches nothing, it acts effectively as disabled matching

Copy link
Member

Choose a reason for hiding this comment

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

Problem is that all the matches we have in Conan for matching packages references are fnmatch based. Introducing now regex for this would be very asymmetric. And for fnmatch, we should probably allow multiple patterns, as it is not as expressive.

Copy link
Member

Choose a reason for hiding this comment

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

Lets merge it, but keep it not documented while we finalize the UX, but there are some template changes that are useful for other PRs.

@memsharded memsharded merged commit 2f7de1b into conan-io:develop Mar 29, 2021
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.

[feature] MSBuildDeps generator: CAExcludePath
2 participants