Add support for passing defines to mig [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 4 comments![]( "Open in Gerrit")
# Python 3: use tempfile.TempDirectory insteadCan do this now (but not in this change).
parser.add_argument('--defines',This is the only comment that you should address in this change:
Since you can only give `--defines` a single define, it should be named `--define` (and the `help=` string should be edited accordingly).
help='Additional defines (may appear multiple times)')There are two ways that I’m not in love with how this works anymore.
1. A lot of these add_argument things are completely passive, and it could make more sense to not build in any special knowledge of them but, rather, just allow this to be invoked as `mig_gen.py --clang-path=… --mig-path=… …/mach_exc.defs …/mach_excUser.c …/mach_excServer.c …/mach_exc.h …/mach_excServer.h -- -I… -I…/compat/mac -I…/compat/ios -DMACH_EXC_SERVER_TASKIDTOKEN -DMACH_EXC_SERVER_TASKIDTOKEN_STATE`. You see that `--` in there? It’d help keep this passive, without having to plumb everything that we need it to pass. \
\
I’m not _entirely_ sure that this will be an improvement, because it’s possible that this code can only avoid having special knowledge of `--include` and `--define`, and will probably need knowledge of at least some of the rest (at least `--arch`, it seems). But it’d _probably_ be better for `--include` and `--define` at least, and there may further code reduction in some of the others from this strategy.
and
generate_interface(parsed.defs, interface, parsed.include, parsed.defines,
parsed.sdk, parsed.clang_path, parsed.mig_path,
parsed.migcom_path, parsed.arch)(continued)
2. This is unwieldy. Most of these should be keyword-only arguments, many with default values.
But you’re just following the status quo, so I don’t think you should make either of these changes here. If you wanted some follow-up cleanup for a slow day, though…
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 5 comments![]( "Open in Gerrit")
Will collect and do the rest in a followup. Only change here is s/defines/define.
ptal!
Can do this now (but not in this change).
will do with other followup work!
This is the only comment that you should address in this change:
Since you can only give `--defines` a single define, it should be named `--define` (and the `help=` string should be edited accordingly).
Done
There are two ways that I’m not in love with how this works anymore.
1. A lot of these add_argument things are completely passive, and it could make more sense to not build in any special knowledge of them but, rather, just allow this to be invoked as `mig_gen.py --clang-path=… --mig-path=… …/mach_exc.defs …/mach_excUser.c …/mach_excServer.c …/mach_exc.h …/mach_excServer.h -- -I… -I…/compat/mac -I…/compat/ios -DMACH_EXC_SERVER_TASKIDTOKEN -DMACH_EXC_SERVER_TASKIDTOKEN_STATE`. You see that `--` in there? It’d help keep this passive, without having to plumb everything that we need it to pass. \
\
I’m not _entirely_ sure that this will be an improvement, because it’s possible that this code can only avoid having special knowledge of `--include` and `--define`, and will probably need knowledge of at least some of the rest (at least `--arch`, it seems). But it’d _probably_ be better for `--include` and `--define` at least, and there may further code reduction in some of the others from this strategy.and
will do in a followup
generate_interface(parsed.defs, interface, parsed.include, parsed.defines,
parsed.sdk, parsed.clang_path, parsed.mig_path,
parsed.migcom_path, parsed.arch)(continued)
2. This is unwieldy. Most of these should be keyword-only arguments, many with default values.
But you’re just following the status quo, so I don’t think you should make either of these changes here. If you wanted some follow-up cleanup for a slow day, though…
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add support for passing defines to mig
This change updates the `mig.py` wrapper to accept `--define`
arguments, which are passed as `-D` options to the underlying `mig`
tool.
This is used to define `MACH_EXC_SERVER_TASKIDTOKEN` and
`MACH_EXC_SERVER_TASKIDTOKEN_STATE` when building the Mach exception
server on iOS. These definitions are required to generate stubs for
`mach_exception_raise_identity_protected` and
`mach_exception_raise_state_identity_protected`.
- M util/BUILD.gn
- M util/mach/mig.py
- M util/mach/mig_gen.py
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
generate_interface(parsed.defs, interface, parsed.include, parsed.defines,
parsed.sdk, parsed.clang_path, parsed.mig_path,
parsed.migcom_path, parsed.arch)Justin Cohen(continued)
2. This is unwieldy. Most of these should be keyword-only arguments, many with default values.
But you’re just following the status quo, so I don’t think you should make either of these changes here. If you wanted some follow-up cleanup for a slow day, though…
will do in a followup!
happy thanksgiving: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/7205874
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |