Assorted fixes for building MPV on Win32 - #16164
Conversation
Later error checking test `if self.rule`, which won't work if rule is unset to begin with.
The length calculation and threshold checks depend on self.elems being completely populated (specifically by ARGS), which is pointless *after* the check is performed.
Fixes the CMake module missing the output of libjxl's `tool_version_git` (because it's generated by an execute_process, and not by the target itself).
|
One bit I forgot to add -- according to the Nasm release notes we should consider blocking all versions < 3.02, as using |
| self.elems: T.List[T.Tuple[str, T.List[str]]] = [] | ||
| self.all_outputs = all_outputs | ||
| self.output_errors = '' | ||
| self.rule: NinjaRule | None = None |
There was a problem hiding this comment.
Later code assumes that it cannot be None, so this is type-unsafe.
The current intended use of this code is that any time a NinjaBuildElement is created, NinjaBackend.add_build(self, build) is called, and sets build.rule = self.ruledict[build.rulename].
The type system doesn't have a good way to describe "the function isn't completely initialized by __init__()" but saying that the property is nullable isn't the correct way to handle this. At least not without more work. Nullable != "uninitialized".
Since commit 739e86f (your previous commit) all call sites for self.rule.* properties occur after a code flow that checks self._should_use_rspfile but I'm not convinced that's actually a good way to uphold this invariant. We could easily add other code accessing self.rule by accident etc.
Perhaps we should be setting it in __init__ rather than adding it after the fact.
There was a problem hiding this comment.
The type system doesn't have a good way to describe "the function isn't completely initialized by init()" but saying that the property is nullable isn't the correct way to handle this. At least not without more work. Nullable != "uninitialized".
We have one in mesonlib:
rule: mesonlib.late_property[NinjaRule] = mesonlib.late_property()
A bit of a mouthful to declare it, but it works and it allows removing the if self.rule statement in _should_use_rspfile.
Setting it in __init__ requires passing the NinjaBuild, which is a largeish change and the late_property alternative is easier.
There was a problem hiding this comment.
Right, thanks. I forgot we added that!
| # NinjaRule.should_use_rspfile counts element.elems too, which will | ||
| # exceed the RSP threshold only after added | ||
| if self.ninja.should_use_rspfile(element) and compiler.rsp_file_syntax() == RSPFileSyntax.NASM: | ||
| element.remove_item('ARGS') |
There was a problem hiding this comment.
This seems correct -- calculating this outside would be a waste of time, anyway.
|
Commit messages should be reflowed to break lines at ~80 chars. |
| for i in args: | ||
| if i in magic_keys: | ||
| if i == 'OUTPUT': | ||
| if i in ('OUTPUT', 'BYPRODUCTS'): |
There was a problem hiding this comment.
pylint asks to use {'OUTPUT', 'BYPRODUCTS'}.
| self.elems.append((name + '_UNQUOTED', elems)) | ||
|
|
||
| def remove_item(self, name: str) -> None: | ||
| self.elems[:] = [e for e in self.elems if e[0] != name] |
There was a problem hiding this comment.
Please make self.elems a dictionary instead, then you can just use del self.elems[name].
As an extra benefit add_item can also check if the name is already present and raise a MesonBugException.
3 participants
Hi all,
I'm coming again here from https://gitlab.freedesktop.org/gstreamer/meson-ports/ffmpeg/-/work_items/75, where I've been tracing consistent failures of Meson's response file generation for Nasm when building MPV.
I'm submitting this as a single MR because I ran into three different bugs when tracking the original issue. These are:
NinjaBuildElement.ruleis not a nullable property, yet it is only set towards the end of its creation:meson/mesonbuild/backend/ninjabackend.py
Line 489 in c9bb03c
which means that any attempts to debug its value or use it through
NinjaBuildElement._should_use_rspfilebefore setting will fail with anAttributeErrorinstead of raising the proper exception.My proposed fix is to make it
Noneso it will at least yield a proper access error or allow the existing exception check to raise.The actual bug in the Nasm response generation test: I found a sort of TOCTOU issue regarding when I check the threshold, the standard check for
use_rspfileinNinjaBuildElement.writetests against the complete set ofelemsin theNinjaBuildElement, which at that time includes the completeARGSfornasm_COMPILER. But my original test is done against the initial (200 chars-ish) set of arguments, not the complete (4k chars) list, which yields a false negative. The result is that Meson still attempts to codegen a brokennasm_COMPILER_RSPrule whose contents will always be rejected by Nasm.My fix is to insert the
ARGSas expected, but then determined if a Nasm-specific response file needs to be generated, and once it's confirmed, remove the item and codegen the response file.The mpv build, when using libjxl from Git, fails with a missing
LIBJXL_VERSIONmacro which ought to come fromtool_version_git.h. Unlike others, the correspondingadd_custom_targetusesBYPRODUCTSto specify the output, as it's been pregenerated by the configure step.https://github.com/libjxl/libjxl/blob/aea3a06e281fdee13e04815bfbf4f4132e7f59ea/tools/CMakeLists.txt#L106
This key is not handled by Meson, which this MR adds.
All feedback is appreciated.