Page MenuHomePhabricator

[openmp][cmake] `CMAKE_INSTALL_BINDIR` usage should not be quoted
ClosedPublic

Authored by Ericson2314 on Jan 28 2022, 10:00 PM.

Details

Summary

As @mstorsjo wrote in https://reviews.llvm.org/D117945#inline-1132920 :

This change seems to have broken one aspect: When doing `ninja

install` I now get a warning saying `Error copying file "libomp.dll" to
"libiomp5md.dll"., and libiomp5md.dll` isn't installed.

I believe the reason is that the inline cmake snippet is written to

runtime/src/cmake_install.cmake and then executed on install, but on
install, ${CMAKE_INSTALL_BINDIR} isn't set (as GNUInstallDirs isn't
included there). Should this maybe expand ${CMAKE_INSTALL_BINDIR}
right here instead of deferring it to the install cmake, or what's the
right course of action?

I agree that is the right course of action. We also agreed to restore the CMAKE_INSTALL_PREFIX that was there before, too.

Diff Detail

Event Timeline

Ericson2314 created this revision.Jan 28 2022, 10:00 PM
Ericson2314 requested review of this revision.Jan 28 2022, 10:01 PM
Herald added a project: Restricted Project. · View Herald Transcript

Oops changed too much

This solution turns out not to work. In this case, ${CMAKE_INSTALL_BINDIR} expands to plain bin here, which doesn't work (as that directory is meant to be relative to the install prefix. Maybe "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}?

Fix more properly

Oh yes oops. It had the \${CMAKE_INSTALL_PREFIX}/ before, so I just put that back right the way it was.

mstorsjo accepted this revision.Jan 29 2022, 12:50 PM

LGTM, this works.

This revision is now accepted and ready to land.Jan 29 2022, 12:50 PM
Ericson2314 edited the summary of this revision. (Show Details)Jan 29 2022, 3:52 PM