compilers: refactor sanity checking code #14775
Conversation
dcbaker
force-pushed
the
submit/refactor-sanity-checking
branch
2 times, most recently
from
28efef4 to
227f598
Compare
dcbaker
force-pushed
the
submit/refactor-sanity-checking
branch
from
227f598 to
a3b5eab
Compare
|
The test failure is strange, but apparently repeatable. |
|
It's failing on every single build, I think that's related to the PR about using pip or an equivalent tool. |
dcbaker
force-pushed
the
submit/refactor-sanity-checking
branch
from
a3b5eab to
666b1c3
Compare
The goal is to reduce code duplication, and allow each language to implement as little as possible to get good checking. The main motivation is that half of the checks are fragile, as they add the work directory to the paths of the generated files they want to use. This works when run inside mesonmain because we always have an absolute build directory, but when put into run_project_tests.py it doesn't work because that gives a relative build directory.
dcbaker
force-pushed
the
submit/refactor-sanity-checking
branch
from
666b1c3 to
9fc2bb0
Compare
| @@ -55,8 +55,11 @@ def get_optimization_args(self, optimization_level: str) -> T.List[str]: | |||
| def get_output_args(self, outputname: str) -> T.List[str]: | |||
| return [] | |||
|
|
|||
| def sanity_check(self, work_dir: str, environment: 'Environment') -> None: | |||
| return None | |||
| def _sanity_check_compile_args(self, env: Environment, sourcename: str, binname: str) -> T.List[str]: | |||
There was a problem hiding this comment.
How is that wrong? The MissingCompiler needs to have implementations of those methods for the type checker, otherwise mypy/pyright will notice that we call abstract methods.
| @@ -96,6 +96,14 @@ def get_depfile_suffix(self) -> str: | |||
| def get_dependency_gen_args(self, outtarget: str, outfile: str) -> T.List[str]: | |||
| return ['-MD', outfile, '-MQ', outtarget] | |||
|
|
|||
| def _sanity_check_compile_args(self, env: Environment, sourcename: str, binname: str) -> T.List[str]: | |||
There was a problem hiding this comment.
Creating so many new instantiations of a do-nothing function purely to quell the abstractmethod error when a class doesn't actually make use of the function in the first place -- because it overrides the method that might have called it -- tells me that this design is too ornate and is doing the wrong thing.
There was a problem hiding this comment.
The problem for all of the assembler classes don't have any sanity checking implemented for them, and the old design made it really easy to allow for that. The reason all of these are filled out is that they're broken by design, but I don't have the expertise to write the assembly code that needs to be checked. So all of these are in fact, pointing out a flaw in the old design that would be obvious with the new design and caught in review.
There was a problem hiding this comment.
@eli-schwartz. I've added an implementation of sanity checking for NASM to point out what I mean. Some of that is being done through shared checking because the way we handle assembly just leans that way. See if that looks better to you (and explains why we need so many do nothing implementations.
| default_ext = lang_suffixes[self.language][0] | ||
| template = f'sanity_check_for_{self.language}' | ||
| sourcename = f'{template}.{default_ext}' | ||
| binaryname = f'{template}{"_cross" if self.is_cross else ""}.exe' |
There was a problem hiding this comment.
Okay, this feels like pretty obvious way to write this code without lots of repetition, what would you suggest?
Such as NASM and MASM. These compilers don't actually do any linking, the just produce object files. As such, we need the C compiler to link the program in order to make things like sanity_check work.
Since assemblers need a C (or similar compiler) to do the linking, a special sanity checking implementation is required.
I have only tested the Linux implementation, but the macOS and Windows implementations will go through CI.
The goal is to reduce code duplication, and allow each language to implement as little as possible to get good checking. The main motivation is that half of the checks are fragile, as they add the work directory to the paths of the generated files they want to use. This works when run inside mesonmain because we always have an absolute build directory, but when put into run_project_tests.py it doesn't work because that gives a relative build directory.
This work was based on #14758, in which I realized that the implementation of sanity_check for Cuda was less than ideal. This is still not fully ideal, as there's a lot of duplication with
compiles, but the amount of work required to makecompilesgeneric enough for this is large, and I really want to get back to other things, so an improvement is an improvement. This also allows me to get to the point that the above mentioned PR works correctly for all languages, and that is enough.This PR seems like it should have a pretty big reduction in LOC, but doesn't. There's a few reasons for that: better doc strings is one, but the biggest is that for many languages the implementation was so basic and limited that the hooks took as many or more lines than the original implementation did. They are benefiting less in cleanup and more in enhanced functionality.