Ensure private directory exists for custom targets #13196
Conversation
|
Can you add a test for this? |
mesonbuild/backend/ninjabackend.py
Outdated
| try: | ||
| if isinstance(target, build.BuildTarget): | ||
| if isinstance(target, (build.BuildTarget, build.CustomTarget)): | ||
| os.makedirs(self.get_target_private_dir_abs(target)) | ||
| except FileExistsError: | ||
| pass |
There was a problem hiding this comment.
While you're here... this could be simplified to:
if isinstance(target, (build.BuildTarget, build.CustomTarget)):
os.makedirs(self.get_target_private_dir_abs(target), exist_ok=True)There was a problem hiding this comment.
Glad you suggested that I do that, just pushed up that change.
|
Added a test case that fails without this change. |
| @@ -0,0 +1,14 @@ | |||
| project('277 custom target private dir') | |||
|
|
|||
| python = find_program('python3', required: true) | |||
| project('277 custom target private dir') | ||
|
|
||
| python = find_program('python3', required: true) | ||
| args = [ |
There was a problem hiding this comment.
I would just inline this.
| custom_target( | ||
| 'check-private-dir', | ||
| command: [python, args, '@PRIVATE_DIR@'], | ||
| output: 'check-private-dir', |
There was a problem hiding this comment.
Might have your little script actually create this file just for completeness
There was a problem hiding this comment.
Sounds good, updated the test case with feedback above along with a print and capture: true so this file actually gets created.
There was a problem hiding this comment.
I think it should create the file directly instead of using capture: true since that kwarg will cause the command line for the custom target to be saved into a second (python) script, which meson internally generates to handle the capturing part.
It means the test case runs two python programs instead of one, and contributes a bit more to the time that it takes to complete the test run on systems with slow forking, such as windows
Admittedly the overhead of adding an entire new test case directory is larger than forking an extra python copy, and so is the overhead of detecting MSVC (which this case doesn't do but many other unittests do a little too frequently). Still, I'm not sure there's a good solution at the moment for those.
There was a problem hiding this comment.
The test case truly does not need to the create the output file to test the functionality of this fix. I feel like if I add more code to this to write out that file (instead of just using capture, which itself isn't even necessary for the test) it would detract from focusing on the actual reduction of what this is meant to test.
If you disagree though, please let me know and I'll be happy to update with writing to @OUTPUT@ to hopefully get this merged.
Edit: Updated to use @OUTPUT@ 😀
|
For some reason I thought that ninja was already supposed to create these... Does this mean that every single custom_target will also get a custom_target.p/ directory created? We do NOT want that unless it's actually getting used. |
mesonbuild/backend/ninjabackend.py
Outdated
| def generate_target(self, target): | ||
| try: | ||
| if isinstance(target, build.BuildTarget): | ||
| os.makedirs(self.get_target_private_dir_abs(target)) | ||
| except FileExistsError: | ||
| pass | ||
| if isinstance(target, (build.BuildTarget, build.CustomTarget)): | ||
| os.makedirs(self.get_target_private_dir_abs(target), exist_ok=True) |
There was a problem hiding this comment.
Yeah, please do not unconditionally do this for all targets, it would be better to only create the directory if @PRIVATE_DIR@ appears in its command: kwarg.
There was a problem hiding this comment.
Ok, took a different approach where it's only created when replacing @PRIVATE_DIR@. This has the added advantage of working across all backends.
|
Can you squash this down into two commits -- one for the main change and one for the exist_ok cleanup? |
appden
force-pushed
the
create-custom-target-private-dir
branch
from
0067b59
to
69253c3
Compare
|
Force pushed squashed commits |
Some custom target commands will expect the `@PRIVATE_DIR@` to already exist, such as with `make -C @PRIVATE_DIR@ ...`
appden
force-pushed
the
create-custom-target-private-dir
branch
from
69253c3
to
a1902f7
Compare
|
Nice work on this. |
|
@eli-schwartz kindly requesting another review, thanks! |
|
Gentle ping for another review, thanks @eli-schwartz @tristan957 |
Some custom target commands will expect the
@PRIVATE_DIR@to already exist, such as withmake -C @PRIVATE_DIR@ ...