De-duplicate argument validation in the build module #14133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Interpreter has been merging gui_app and win_subsystem for a while, but now with the typed kwargs we can feel more comfortable that gui_app wont get passed through beneath us
…used" This reverts commit 93c11f2. We're going to use it again in the next commit
This just does what would happen later, but with the potential to fail in obscure way.
dcbaker
force-pushed
the
submit/build-cleanup-argument-validation
branch
from
078abb2 to
0886be4
Compare
This includes cleaning up some of the type handling to account for cleanups that are done at the DSL level.
We have type checking, we also have interpreter level validation from the DSL, we don't need this.
There's really no reason to not do ths in the `_extract_pic_pie` helper, it only increases the chance of us not doing this when we should.
It's only valid to use `b_staticpic` with `pic`, and `b_pie` with `pie`, so just key it off the argument
This logic is specific to StaticLibrary and to Executable, so put the logic there. While we're at it, make the type checking more rigid.
I've left the workaround for the install/build_by_default interaction for later, that's a more complicated issue to prove, since the Interpreter does that automatically, so we need to audit non-Interpreter interactions
There is some validation going on here that cannot (currently) be done the KwargInfo validators, namely that the files exist. Additionally, BuildTarget.extra_files is doing some membership checking for dups, which means it should probably be an OrderedSet instead of a list.
dcbaker
force-pushed
the
submit/build-cleanup-argument-validation
branch
from
0886be4 to
241d433
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series adds TypedDicts for the various
BuildTargetderived classes, and then rips out all of the validation in the build layer that is already done in the Interpreter layer. This does not attempt to mess with any validation that is not done at the interpreter layer, however.There is also a bit of fixing of the types in a few places, namely the explicit conversion of
installto bool, rather than doing the same conversion implicitly later.