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 : provide Path accesssors #11585

Merged
merged 1 commit into from Jul 8, 2022
Merged

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Jul 7, 2022

closes: #11304

Changelog: Feature: Provide Path accessors in Conanfile.
Docs: omit

  • 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.

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.

Please add some basic integration test that checks these added methods.

@property
def install_folder(self):
# FIXME: Remove in 2.0, no self.install_folder
return self.folders.base_install

@property
def install_path(self) -> Path:
Copy link
Member

Choose a reason for hiding this comment

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

This one disappears in 2.0, better remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@property
def imports_folder(self):
return self.folders.imports_folder

@property
def imports_path(self) -> Path:
Copy link
Member

Choose a reason for hiding this comment

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

This one disappears in 2.0, better remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -129,6 +149,11 @@ def imports_folder(self):

return os.path.join(self._base_imports, self.imports)

@property
def imports_path(self) -> Path:
Copy link
Member

Choose a reason for hiding this comment

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

This one disappears in 2.0, better remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -1,7 +1,7 @@
import os

from conans.model.new_build_info import NewCppInfo

from pathlib import Path
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that the Folders xxxx_folder are not intended nor documented for users yet.
If anything we want to replace the xxx_folder for the path alternatives. But this is not necessary in this PR, as this is intended only for user recipes which is always self.xxxx_path, and not self.folders.xxxx_path.

So maybe leave this file untouched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I left it untouched

Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4
Copy link
Contributor Author

SSE4 commented Jul 7, 2022

Please add some basic integration test that checks these added methods.

added (updated an existing test with more checks)

@memsharded memsharded added this to the 1.51 milestone Jul 7, 2022
@ytimenkov
Copy link
Contributor

Out of curiosity: why not to change existing fields but add new ones? What are the potential issues with changing the type?

@SSE4
Copy link
Contributor Author

SSE4 commented Jul 7, 2022

Out of curiosity: why not to change existing fields but add new ones? What are the potential issues with changing the type?

it may break existing recipes. e.g. the one doing something like isinstance(self.package_folder, str) or similar.

@ytimenkov
Copy link
Contributor

it may break existing recipes. e.g. the one doing something like isinstance(self.package_folder, str) or similar.

Thank you for quick reply. That part I have figured out. Was more interested if there are recipes which do this and why.

In my limited python experience calling isinstance is discouraged. The most common check for type I know is str vs list of strings (where str is converted to a single-element list, but the check is for list, because it can be something else) for caller convenience. Other examples str vs bytes - just call encode, and there is is None for optionals.

@SSE4
Copy link
Contributor Author

SSE4 commented Jul 7, 2022

I don't know any recipe doing it out of my mind. still, it's possible, some recipe does some_python_func(self.package_folder) where some_python_func calls is_instance under the hood.

nethertheless, there could be other breaking cases, e.g.:

var = self.package_folder.replace("foo", "bar")

will crash if self.package_folder is pathlib.Path, but it will work if it's just a regular string. and replace for sure is done in many recipes.
an example: https://github.com/conan-io/conan-center-index/blob/c102ed11805c88f636198d1feec892eea8c20579/recipes/bison/all/conanfile.py#L156

for sure it could be changed in 2.0, but for 1.x I would say it's too risky.

@memsharded
Copy link
Member

I am merging this because it is done, but after talking to the team, please let me clarify:

  • We really, really need to stop adding new things to Conan 1.X. Only add things that are strictly necessary for the migration to 2.0. This was clearly not necessary.
  • We will not document it yet, now things added to 1.X basically need to be documented twice, and in 2.0 is complicated because we are still discussing many things around the docs.
  • This will be there as experimental and not supported yet. If it becomes a maintenance or support burden, we will remove it, but we will not invest more time in things that could derive from this feature, until Conan 2.X

Thanks for understanding

@memsharded memsharded merged commit 4ed1bee into conan-io:develop Jul 8, 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.

[feature] add self.*_path properties that return pathlib.Path
4 participants