Add file at cache level #8
Conversation
There was a problem hiding this comment.
I think there are a couple of things I would see improved:
- The interface
require_namespaceis a bit weird. Maybe it could be hidden, and if any, it would have this argument in the constructor/creator to raise an error if package namespaces are used in the file in a repository - The path manipulations on
work_on_item()are difficult to understand. Would need some explanation, understanding what problem they are trying to solve. - Conan uses extensively the
fnmatchpattern based approach, so*is not just checked as is a wildcard => match everything. But the namespaces are actually patterns that are matched against package references. I think it would be more consistent to use that approach. - The priority should be updated to account for what we discussed.
| def _load_editables_cpp_info(self): | ||
| editables_path = self._client_cache.default_editable_path | ||
| if os.path.exists(editables_path): | ||
| return EditableCppInfo.create(editables_path, require_namespace=True) |
There was a problem hiding this comment.
required_namespace maybe not very intuitive. I guess that means that repository layout files will error if they find some [LibA:includedirs] line. Maybe allow_package_prefix?
conans/client/installer.py
Outdated
| if os.path.exists(package_layout_file): | ||
| editable_cpp_info = EditableCppInfo.create(package_layout_file, | ||
| require_namespace=False) | ||
| editable_cpp_info.apply_to(node.conanfile.name, |
There was a problem hiding this comment.
You might want to factor out the call to apply_to() and do a single call (restrict the if-else to obtain a editable_cpp_info, then apply it.
There was a problem hiding this comment.
I will consider it when implementing the third block in the if-elif-else, the one corresponding to the package_info() data.
| def _work_on_item(value, base_path, settings, options): | ||
| value = re.sub(r'\\\\+', r'\\', value) | ||
| value = value.replace('\\', '/') | ||
| isabs = ntpath.isabs(value) or posixpath.isabs(value) |
There was a problem hiding this comment.
This is still extremely weird. I need to understand this path manipulation, probably deserves whiteboard.
There was a problem hiding this comment.
Tests are passing without the first line, I'm removing it.
About the third line (isabs = ....):
In the general use case there will be one file containing paths for Windows and paths for Linux, and for every non-absolute path we want to calculate the absolute one (prepending the base_path varaible). This parsing and normalization is done in Windows and Linux (using the same file) and I think it is cleaner if we identify which paths are already absolute or we will come up with things like this:
/this/is/my/base/path/C:/it/was/an/absolute/windows/path
I'm even thinking about removing paths that do not correspond to the running OS.
| ret = defaultdict(lambda: {k: [] for k in cls.cpp_info_dirs}) | ||
| for section in parser: | ||
| if ':' in section: | ||
| namespace, key = section.split(':', 1) |
There was a problem hiding this comment.
namespace is actually a package name?
There was a problem hiding this comment.
If required_namespace I think finding a : should fail.
There was a problem hiding this comment.
Read the comment below: #8 (comment)
|
Points still pending:
I'm trying not to raise if the file contains (yes/no) scoped sections when those sections are expected (with/out) scope. The reason for that is that this file (remember that we don't know still where this information is going to be) may contain additional information with more sections that can have (yes/no) scope. About the naming: I may change this variable name to
Read the comment here: #8 (comment)
This is open to discussion: inside this profile-like file, do we allow to have a pattern matching or just the two options: explicit (equals to the package name) versus Does it make sense to add full reference (with or without pattern matching) or it is enough to have the package name?
Ok, but please read again: conan-io#4142 |
No description provided.