Disallow tools.os_info and platform in requirements/build_requirements/config_options/configure/validate #320
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
uilianries
previously approved these changes
Aug 6, 2021
SSE4
previously approved these changes
Aug 19, 2021
madebr
commented
Aug 19, 2021
Thanks. I used I made a last minute change. |
SpaceIm
reviewed
Aug 19, 2021
|
not much |
|
@madebr seems like there are conflicts now |
eb83ccd to
57d0681
Compare
I rebased the pr. |
SSE4
previously approved these changes
Aug 19, 2021
madebr
commented
Aug 19, 2021
uilianries
previously approved these changes
Aug 20, 2021
SSE4
previously approved these changes
Aug 20, 2021
|
I have run a hook check with the changes in this PR to see what recipes would fail. Here is the list: We'd probably need to work on some of them before merging this new check. |
|
Yes, please, let's try to fix some of them before activating the hook. Opening those PRs will also help us to give some visibility to this issue. |
This was referenced Aug 23, 2021
|
If anyone is interested to help in porting recipes, the following code automatically replaces usage of Execute it as import ast
import collections
import pathlib
import sys
import typing
Location = collections.namedtuple("Location", ("line", "column", "line_end", "column_end"))
BuildInfo = collections.namedtuple("BuildInfo", ("loc", "what", "func", "ast"))
class BuildInfoVisitor(ast.NodeVisitor):
INVALID_METHODS = (
"config_options",
"configure",
"build_requirements",
"requirements",
"validate",
)
def __init__(self):
self.invalids: typing.List[BuildInfo] = []
self.function_def_stack = []
ast.NodeVisitor.__init__(self)
def visit_FunctionDef(self, node):
self.function_def_stack.append(node.name)
self.generic_visit(node)
self.function_def_stack.pop()
def visit_Attribute(self, node):
methods_stack_no_build_info_allowed = [fdef for fdef in self.function_def_stack if fdef in self.INVALID_METHODS]
if methods_stack_no_build_info_allowed:
if isinstance(node.value, ast.Attribute) and isinstance(node.value.value, ast.Name) and node.value.attr == "os_info" \
and node.value.value.id == "tools":
self.invalids.append(BuildInfo(Location(node.lineno, node.col_offset, node.end_lineno, node.end_col_offset), "tools.os_info", methods_stack_no_build_info_allowed[0], node))
elif isinstance(node.value, ast.Name) and node.value.id == "platform":
self.invalids.append(BuildInfo(Location(node.lineno, node.col_offset, node.end_lineno, node.end_col_offset), "platform", methods_stack_no_build_info_allowed[0], node))
self.generic_visit(node)
def check_source(path: pathlib.Path):
node = ast.parse(path.read_text())
visitor = BuildInfoVisitor()
visitor.visit(node)
return visitor.invalids
def fix_source(path: pathlib.Path, invalids: typing.List[BuildInfo]):
text_lines = path.read_text().splitlines()
for invalid in invalids:
if invalid.loc.line != invalid.loc.line_end:
raise Exception("Cannot handle multi-line replacement")
if invalid.what == "tools.os_info":
if invalid.ast.attr == "is_windows":
repl = "self._settings_build.os == \"Windows\""
else:
raise Exception("{}: unknown \"{}\"".format(path, invalid.ast.attr))
line = text_lines[invalid.loc.line-1]
new_line = line[:invalid.loc.column] + repl + line[invalid.loc.column_end:]
text_lines[invalid.loc.line-1] = new_line
else:
print("{}: Unhandled {}".format(path, invalid.what))
new_text = "\n".join(text_lines)
if new_text[-1] != "\n":
new_text += "\n"
return new_text
def main():
path = pathlib.Path(sys.argv[1])
invalids = check_source(path)
fix = fix_source(path, invalids)
path.write_text(fix)
if __name__ == "__main__":
main() |
4 tasks
4 tasks
cfebad2 to
b674eed
Compare
b674eed to
485bc2b
Compare
4 tasks
|
Could this be added to the bincrafters convention tool? #320 (comment) |
|
Yes, but it must be improved because it does not create a |
This was referenced Sep 8, 2021
Merged
Merged
| @@ -1,3 +1,5 @@ | |||
| import ast | |||
There was a problem hiding this comment.
Is this like javascript/Typescript AST??? 😱
There was a problem hiding this comment.
It's very basic. See ast.
It allows walking and transforming an ast.
The only downside is that it doesn't preserve comments.
There was a problem hiding this comment.
I was looking at redbaron yesterday.
It looks very nice to do full source transformation.
That library does preserve full layout.
|
Much better now, thanks for the amazing work fixing those hooks: |
danimtb
approved these changes
Sep 10, 2021
|
time to get this merged. Thank you! |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In recent weeks, c3i infrastructure + cci recipes received a lot of attention regarding cross building:
While doing so, it became clear that the use of
tools.os_infoandplatformis troublesome if host profile != build profile.This is even more important for c3i because the arbiter (=entity that sends builds jobs to linux/windows/macos machines) runs on Linux.
So it becomes important that all checks inside
config_options/configure/validate/requirements/build_requirementsdo not depend on e.g.tools.os_info.is_windowsor theplatformmodule.See e.g. the following comments (and more):
Instead of
tools.os_info.XXX, we/I propose to useself.settings_build(if available).See conan-io/conan-center-index#6371 (comment)
So the following python code:
can be replaced with:
This alternative method, which is cross compile proof, has been successfully used by @SpaceIm in the last days.