[shortfin][build] Improve dependency management for builds #132
Conversation
libshortfin/CMakeLists.txt
Outdated
| # tests | ||
|
|
||
| if(SHORTFIN_BUILD_TESTS) | ||
| if (SHORTFIN_IREE_SOURCE_DIR) |
There was a problem hiding this comment.
We should modernize how IREE does this so it plays nicely with others. The right way to do it would be to teach IREE how to fetch googletest dynamically and then make ours available before including IREE (these work as first one wins).
There was a problem hiding this comment.
I'm not very sure how to do that on the IREE side. Can we namespace the iree googletest dependency? I'm planning on atlest not doing a add_subdirectory on google test in IREE if we are not building tests.
There was a problem hiding this comment.
I agree we should take a look into modernizing IREE. For now I am fine with this but we should add it to the list for future improvements.
libshortfin/CMakeLists.txt
Outdated
| if(SHORTFIN_IREE_SOURCE_DIR) | ||
| set(IREE_BUILD_COMPILER OFF) | ||
| set(IREE_BUILD_TESTS OFF) | ||
| add_subdirectory(${SHORTFIN_IREE_SOURCE_DIR} iree SYSTEM) |
There was a problem hiding this comment.
Let's also only enable backends that we care about: right now just local and amdgpu.
Also, for my own edification, why do you need SYSTEM here?
There was a problem hiding this comment.
Following the discussion on discord, seems like there is a unused function error propagating from IREE and due to -Wno-used-function set. However, just building with IREE_BUILD_COMPILER_OFF shouldn't trigger that error (we don't see it in iree-bare-metal-arm) but I can dig into with @Groverkss tomorrow.
There was a problem hiding this comment.
The error comes because IREE uses -Wno-unused-function and other compilation options, while our CMakeLists.txt doesn't (for now at least). IREE will fail to compile without these flags. I used SYSTEM to isolate IREE's compilation flags to itself. I'm guessing this could also bite us in future if we wanted to use some of our own flags. Not sure whats preferred here.
There was a problem hiding this comment.
Let's go with what you have (at least) for now :)
Groverkss
changed the title
…/Groverkss/dep-manage
This reverts commit 4f10f6e.
…/Groverkss/dep-manage
Add logic to cmake configuration to automatically download dependencies if they cannot be found. Also adds a flag to specify SHORTFIN_IREE_SOURCE_DIR to build iree out-of-tree.