compor/llvm-ir-cmake-utils

get absolute IN_FILE based on SOURCE_DIR of DEPENDS_TRGT

tandf opened this issue ยท 6 comments

tandf commented

Hi compor,

In llvmir_attach_bc_target, to get the absolute path of the in-file, currently get_filename_component is used:

get_filename_component(INFILE ${IN_FILE} ABSOLUTE)

According to the doc, get_filename_component resolves the absolute path based on the CMAKE_CURRENT_SOURCE_DIR variable.

If no base directory is provided, the default base directory will be CMAKE_CURRENT_SOURCE_DIR.

However, the CMAKE_CURRENT_SOURCE_DIR variable doesn't always stand for the path where the DEPENDS_TRGT is defined. To handle the cases where CMAKE_CURRENT_SOURCE_DIR is messed up, I propose to instead explicitly look for the source path of DEPENDS_TRGT, i.e. use the SOURCE_DIR:

This read-only property reports the value of the CMAKE_CURRENT_SOURCE_DIR variable in the directory in which the target was defined.

For normal cases where the CMAKE_CURRENT_SOURCE_DIR variable is not messed up, it should contain the same value as SOURCE_DIR.

I have submitted a pull request for this change, see #28. Please let me know if you have any question. Thanks!

@tandf thanks for the details explanation and sorry for the delay; I've left some minor comments.

tandf commented

@compor Thanks for the comments. I just added two replies to your comments.

Hi again, sorry for being pedantic, but I'm in the unique position of not remembering the implementation very well ;-) so things like that stand out, so why not fix them eh?

tandf commented

Thanks for the comments. I just submitted a new commit for resolving the comments. For the details, please refer to the commit and my comments. Please help me review it when you have time. Thanks!

If it looks good to you, I can combine the two commits into one before merging this pr.

tandf commented

The commits are combined

Addressed by #28
Thanks!