vala: Custom header and vapi name (fix #892) #912
Conversation
|
This should go in after #908 since it touches that code. |
|
@nirbheek If you can cherry-pick and make it suitable for 0.36 that would be really nice. Otherwise I think it's good to keep this open if Jussi decide to release before you're done. |
|
Yes, of course we should keep this open. :) The patch needs some more work too; just that it's a bit pointless to do that right now because it's based on a branch that doesn't have necessary fixes yet. Will do that once that's done. |
arteymix
force-pushed
the
wip/vala-custom-header-and-vapi
branch
3 times, most recently
from
3a2cf3e
to
e500efe
Compare
|
@nirbheek Ok, I just rebased upon your changes Thanks for moving the VAPI out of |
arteymix
force-pushed
the
wip/vala-custom-header-and-vapi
branch
from
e500efe
to
d79e47e
Compare
| @@ -449,6 +451,9 @@ def process_kwargs(self, kwargs, environment): | |||
| if not isinstance(valalist, list): | |||
| valalist = [valalist] | |||
| self.add_compiler_args('vala', valalist) | |||
| if not isinstance(self, Executable): | |||
| self.vala_header = kwargs.get('vala_header', None) | |||
| self.vala_vapi = kwargs.get('vala_vapi', None) | |||
There was a problem hiding this comment.
Why not use kwargs.get('vala_header', self.name + '.h'), etc here? Simplifies the code a bit.
There was a problem hiding this comment.
True, that would put less burden on the backend.
There was a problem hiding this comment.
The patch itself LGTM, but I will note that the tests don't test that the kwargs actually work right now. We should edit this in after we have the ability to install the header and vapi.
... actually, could you merge those two tests into one? Seems a bit overkill to have two separate ones. :)
With that change, I think this can go in.
Move the fallback logic into 'BuildTarget.process_kwargs' to use the target name instead.
arteymix
force-pushed
the
wip/vala-custom-header-and-vapi
branch
from
9c6bb7b
to
114b5b0
Compare
|
I just merged the two test cases into one. I'm not against introducing |
| # Force valac to write the vapi file in the target build dir. | ||
| # Without this, it will write it inside c_out_dir | ||
| args += ['--vapi=../' + base_vapi] | ||
| args += ['--vapi=../' + target.vala_vapi] |
There was a problem hiding this comment.
@nirbheek what's wrong with get_target_dir at this point? Isin't the C header generated in the same folder already?
There was a problem hiding this comment.
Also, we should use os.path.join anyway for portability.
There was a problem hiding this comment.
I don't understand your question. We could use os.path.join here, sure.
There was a problem hiding this comment.
There's two different strategy used to generate a file in what appears to be the same directory (get_target_dir vs ..). It seems to me that the former is more correct, no?
There was a problem hiding this comment.
If it's equivalent, I can patch it to consistently use os.path.join with get_target_dir.
There was a problem hiding this comment.
The two are not the same. get_target_dir() will return the subdir relative to the build root where the target is being built (the "target build dir"), which is not ...
For some reason, valac does not put the generated .h and the .vapi in the same directory. The .h file is placed in the directory as specified by the path, but the .vapi is placed alongside the generated C files in the 'target private directory' (see c_out_dir above) which is a subdir of the target build dir. The .. is a hack to make it output the .vapi in the target build dir.
Try it out and see. You can make it os.path.join('..', target.vala_vapi) if you wish. :)
There was a problem hiding this comment.
Then I need to check if the same apply for vala_gir.
The same is done for the
--girflag in #634.