Fix multi-config generator handling and detection#3315
Fix multi-config generator handling and detection#3315jonasehrlich wants to merge 1 commit intoonnx:mainfrom
Conversation
|
Can one of the admins verify this patch? |
|
@jenkins-droid test this please |
2877b79 to
a334033
Compare
|
Can one of the admins verify this patch? |
|
@jonasehrlich seems like there is an issue with the new changes; do you mind to look into fixing the cause of the error. Let me know if you cannot see the log. I would start to look at the linux amd64, probably more familiar with that machine. Otherwise, maybe close the PR. Thanks |
|
@AlexandreEichenberger The initial job triggered by you failed with Afterwards I rebased the PR as the build log suggests and this latest job fails with: Just from a first look at the pipeline, I would assume you would have to retrigger it. But I am currently rebuilding again to check the build itself |
|
Thanks @jonasehrlich. Ping me with a new "@" message when you are ready, and I will authorize the run again. |
|
@AlexandreEichenberger My local build is done and the macOS build stage in GitHub actions passed as well and the tests are running. Please trigger the Jenkins run again, thank you! Would it make sense to include multi config generator builds in the CI pipeline? Just as a test if these are working? |
|
@jenkins-droid test this please |
|
@AlexandreEichenberger I updated the MR again with a merge commit (through the GitHub UI), so the pipelines are stuck waiting for approval again. Tomorrow morning EU time, I can also do the rebase if that works better. Is there any guideline on what still needs to be done to get this merged? Thanks for the feedback! |
|
I think the PR should be in good shape. We always appreciate fixes for users that do things a bit differently and catches some of the idiosyncrasies of our local build. Thanks. I am adding @tungld also here as I will go away for a break soon. |
|
@jenkins-droid test this please |
94ff2aa to
5ca1e69
Compare
|
Can one of the admins verify this patch? |
|
@AlexandreEichenberger Just saw you retriggered the pipeline, but it failed because the base commit was out-of-date again. I rebased my PR branch. Can you please retrigger the build? |
|
We seems to have an issue with our CIs at this time, which is CI specific as I can build fine on my Mac. I expect to take a few days to get to the root of it. Feel free to ping me again in a few days in case I forget to re-trigger it. Your changes are really out of my regular expertise, but if it works for you (using this new mode) and does not prevent our CIs to work (once they are fixed), will approve. |
Previously, multi-config generators were detected by the value of the `CMAKE_CFG_INTDIR` variable. If a multi-config generator was detected, the CMAKE_CFG_INTDIR was included. This has different values from the $<CONFIG> generator expression into which the ExternalUtil.hpp file is generated for each configuration. This change uses `CMAKE_CONFIGURATION_TYPES` for multi-config generator detection and provides a more robust dependency handling for the generated files. Additionally it uses the `$<CONFIG>` generator expression to set the include directory for multi-config generators. See onnx#3314 Signed-off-by: Jonas Ehrlich <jonas.ehrlich@xneural.ai>
5ca1e69 to
4c779fa
Compare
|
Can one of the admins verify this patch? |
Previously, multi-config generators were detected by the value of the
CMAKE_CFG_INTDIRvariable. If a multi-config generator was detected, the CMAKE_CFG_INTDIR was included.This has different values from the $ generator expression into which the ExternalUtil.hpp file is generated for each configuration.
This change uses
CMAKE_CONFIGURATION_TYPESfor multi-config generator detection and provides a more robust dependency handling for the generated files. Additionally it uses the$<CONFIG>generator expression to set the include directory for multi-config generators.Closes #3314