Fix profiling preparing - #95
Conversation
…re builds which didn't build
formatting message
refactor(configurator): use build systems API refactor(validator): use build systems amphimixis feat(core): update API related with build systems feat(amphimixis): update API related with build systems refactor(general): improve build systems
1d76865 to
12b0526
Compare
| ret_flags.append(f"{"".join(self._attrs_map(flag))}='{value}'") | ||
| return " ".join(ret_flags) | ||
| def build(self, build: Build) -> tuple[int, str, str]: | ||
| """Build via Make |
There was a problem hiding this comment.
add extended function description to reflect difference between build and run building
docs(CMake): update build systems docstrings docs(Make): update build systems docstrings docs(Ninja): update build systems docstrings
| runner_name: str = str(input_config.get("runner", None)).lower() | ||
|
|
||
| if not get_build_system(str(build_system_name)): | ||
| if not (build_system_name := str(_get_analyzed_build_system())): |
There was a problem hiding this comment.
| if not (build_system_name := str(_get_analyzed_build_system())): | |
| if not (build_system_name := _get_analyzed_build_system()): |
str(None) is evaluated to 'None' which is evaluated to True
| if build_system not in build_systems_dict: | ||
| if not (build_system := _get_analyzed_build_system()): | ||
| build_system_name: str = str(input_config.get("build_system")).lower() | ||
| runner_name: str = str(input_config.get("runner", None)).lower() |
There was a problem hiding this comment.
r u sure that "none" value is OK here?
There was a problem hiding this comment.
runner_name = str(None).lower() i mean
There was a problem hiding this comment.
runner with name 'none' will not be found and we will get None from build_systems api functions
There was a problem hiding this comment.
trying to get runner, which you know doesn't exist, is strange
maybe check if not None then get()?
There was a problem hiding this comment.
and check the previous line: it perfoms the same actions for build_system_name but it is written a bit differently
check for consistency
| project.build_system = build_system( | ||
| project, runner(project=project, ui=ui), ui # type: ignore[arg-type] | ||
| ) # type: ignore[assignment] | ||
|
|
There was a problem hiding this comment.
| project.build_system = build_system( | |
| project, runner(project=project, ui=ui), ui # type: ignore[arg-type] | |
| ) # type: ignore[assignment] | |
| project.build_system = build_system( | |
| project, runner(project=project, ui=ui), ui # type: ignore[arg-type] | |
| ) | |
i think types are fucked up
maybe Project.build_system should be superclass, so it can have values of subclasses? (idea from Liskov substitution principle)
maybe make IHighLevelBuildSystem and ILowLevelBuildSystem to be subclasses of BuildSystem?
build_system: IHighLevelBuildSystem = get_build_system(build_system_name)
Project.build_system: BuildSystem = build_system # so there should not be type error
|
/oc review |
|
/oc summary |
PR Review: Fix profiling preparing & logging refactorSummaryThis PR fixes Key Changes
Review Feedback to Address
VerdictCore changes are correct. Fix the docstring and configurator None-handling issues, and the PR is ready to merge. |
2 participants
fix(cli.commands.profile.setup_profiling_environment): check if build has been built
refactor: improve logging in Builder and build systems: add formatting level with build name