Cleanups for IncludeDirs
#15180
Draft
Cleanups for IncludeDirs
#15180
Conversation
This file contains hidden or 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
bonzini
reviewed
Oct 29, 2025
| # -Ipath will add to begin of array. And without reverse | ||
| # flags will be added in reversed order. | ||
| args: T.List[str] = [] | ||
| for p in inc.rel_string_list(self.build_to_src, self.build_dir): |
Member
Author
There was a problem hiding this comment.
That causes the test_internal_include_order test to fail, so I think I ported the comment over incorrectly
Member
Author
There was a problem hiding this comment.
Which doesn't make sense.
I need to think about this one more.
Member
Author
There was a problem hiding this comment.
Okay, after a lot of head scratching and confusion, I have an answer: rel_string_list returns the stringlist in the order that this code expects already, so no reversing required. The reason the previous code reversed the iteration was to get build directory before source directory, which this does by default.
This is really class constant for all dependencies, and by taking it out of the initializer we make the `__init__` call have a more consistent interface.
So we don't create a default name that is overwritten except in the case of appleframeworks. This allows for some cleanup, including deleting some initializers that were only setting the name.
We have type checking that ensures this is a string list already.
It's allowed in the `DependencyKeywordArguments` TypeDict already, so we now have two sources of truth. Additionally, it's often populated by reading from that dict, so we're just doing useless work.
…Dependency It's read out of the `kwargs`, then passed along with the kwargs. We can just get it in the class initializer for no cost
This is always derived from kwargs, so now we have two sources of truth that can go out of sync.
This simplifies a bunch of cases, and likely fixes some annoying bugs in cross compile situations where should have been passing this and weren't.
The goal is to have a single type for candidates that replaces having a mixture of class initializers, partials, and factories with a single wrapper class.
This makes the type checking more reasonable (including fixing an issue that newer mypy is pointing out to us), consolidating special cases, and improving code readability.
Avoid extra method calls and repeating ourselves.
It's now only used to populate the DependencyCandidate, so we can remove it and just calculate the same information from the `type_name` parameter. This reduces code and the number of method calls.
This includes some cleanup to remove unused calculations.
dcbaker
force-pushed
the
submit/cleanup-build-includedirs
branch
from
16eb3ef to
295fed3
Compare
It doesn't represent everything, and dataclass will generate one for us anyway.
It just returns a public attribute, so it's an extra cost for no benefit
Same as previous commit for another method
Same as the previous removal
This is groundwork for the next patch
Which would otherwise prevent the use of generated sources and (in the future) sources from dependencies
dcbaker
force-pushed
the
submit/cleanup-build-includedirs
branch
from
295fed3 to
edc117e
Compare
To prepare for the next patch
This allows us to clean up many instances of the same pattern implemented elsewhere, by consolidating into one.
This adds the ability to remove build directories that don't exist. This doesn't require a second call to reverse() because that was being used to switch the order of the build and src directories, but `rel_string_list` returns them in the expected order already.
dcbaker
force-pushed
the
submit/cleanup-build-includedirs
branch
from
edc117e to
8a38912
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 is built on top of #15178
As I hurtle onward to getting dependencies in ready for having split compile arguments, I've stopped to work on splitting include directories out of compile args. To that purpose I've been working on making the
IncludeDirsobject more flexible, and this is the first leg of that work.There is some just generic cleanups here, but also a lot of fixes for handling
extra_build_dirs. To that point I've attempted to replace all (although one unique instance remains in the VS backend) conversion fromIncludeDirstoList[str]to use methods built into the object itself, and which correctly handle those cases.