Honor includes in partial_dependency of externals dependencies #14318
Conversation
bruchar1
force-pushed
the
partial-dep-keep-includes
branch
from
4b97885 to
736f06e
Compare
| @@ -420,13 +420,14 @@ def get_partial_dependency(self, *, compile_args: bool = False, | |||
| new = copy.copy(self) | |||
| new._id = uuid.uuid4().int | |||
| if not compile_args: | |||
| new.compile_args = [] | |||
| if includes: | |||
| new.compile_args = [c for c in self.compile_args if c.startswith('-I')] | |||
There was a problem hiding this comment.
That's not sufficient, at the very least this needs to be c.startswith(('-I', '/I', '-isystem', '-idir-after')), but even that probably isn't sufficient.
I'm also really, really, hesitant to do this in general, since the only valid case where a we could get a raw include is from pkg-config (or maybe CMake?). I'd tried to fix this previously and ran into pkgconf bugs on mingw: #10122
At the very least I think this should be limited to pkg-config dependencies.
There was a problem hiding this comment.
I was actually thinking about that PR this weekend
There was a problem hiding this comment.
I see... But OTOH, it will not change the behavior if compile_args is true. The only thing it changes is when you request includes but not compile_args.
Not sure about -isystem and -idirafter. Should includes from a dependency include system dirs? For /I, I though it was already replaced by -I by meson, but I may be wrong... The only thing I know is that pkgconf --cflags-only-I will only keep arguments beginning with -I.
There was a problem hiding this comment.
I proposed this because I get caught this morning, trying to use the includes from a dependency. I forgot it was only working for internal dependencies...
There was a problem hiding this comment.
My concern is it changes the behavior of a declare_dependency()
x = declare_dependency(compile_args : ['-Dfoo', '-Ibar']).partial_dependency(include_directories : true)Currently gives an empty declare_dependency. This is what we intend, we don't support putting -I flags in compile_args, you get to keep the pieces if you do that.
There was a problem hiding this comment.
This wouldn't change, since my changes only apply to external dependencies. Behavior of internal_dependencies remains the same.
There was a problem hiding this comment.
hmmm, I'm still not sure. I'll have a look at what ends up here and then go from there.
Either way a test would be good. There might be something in that PR I linked you could steal.
bruchar1
force-pushed
the
partial-dep-keep-includes
branch
from
736f06e to
ae506e9
Compare
|
I think, generally speaking, extracting includes from a dependency is a bad idea. For example glib has -mms-bitfields in the compile-time arguments and extracting includes would leave that out, thus compiling with a different ABI. In fact, I would rather deprecate |
This is different than the approach in #10122, as it doesn't interfere with the order of cflags. It only return a subset of them, in the case the dependency is an external dependency, and
partial_dependencyrequestsincludesbut notcompile_args.