Skip to content

Conversation

@Reflexe
Copy link
Member

@Reflexe Reflexe commented Aug 12, 2019

No description provided.

lukas-w and others added 10 commits July 18, 2019 08:45
Replaces the hard-coded library paths by a method based on CMake's
GetPrerequisites module which recursively finds a binary file's linked
libraries. Advantage: Potentially works on any system without adaption as
long as CMake supports it, so it could be used to create portable Linux
packages as well. Disadvantage: "Potentially".
Also enables deploying dependencies for RemoteZynAddSubFX.
* Add missing entries into the Windows exclude list
* Respect the case insensitivity of Windows
* Fix incorrect `lib` prefix for plugins on Windows
For some reason `MESSAGE(STATUS)` doesn't work on install time.
Fixes MSVC build error
@Reflexe Reflexe self-assigned this Aug 12, 2019
@Reflexe Reflexe force-pushed the cmake/install-refactor branch from c2f2c9e to 6e1d2ff Compare August 12, 2019 16:08
@Reflexe Reflexe force-pushed the cmake/install-refactor branch from 6e1d2ff to d937775 Compare August 12, 2019 16:18
@PhysSong PhysSong self-requested a review August 13, 2019 07:43
@Reflexe
Copy link
Member Author

Reflexe commented Aug 20, 2019

I don't get it; for some strange reason cmake tries to write each filelists.txt 9 times with appveyor.
https://ci.appveyor.com/project/Lukas-W/lmms/builds/26826636/job/5rob3k12jdr07f83

maybe we add_subdirectory 9 times somehow?

Ok, according to sakra/cotire#120 it probably has something to do with those generator expressions.

We could try to install the artifact into a temporary directory or just install the artifact and remove other installations.

if(IS_DIRECTORY ${lib_dir}/bin)
list(APPEND LIB_DIRS ${lib_dir}/bin)
endif()
endif()
Copy link
Member

@PhysSong PhysSong Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trick(adding ../bin and ./bin) should be moved to cmake/install/InstallTargetDependencies.cmake, after line 32. It was here to support non-standard locations, ex. libwinpthread-1.dll in Travis-CI builds.

Copy link
Member Author

@Reflexe Reflexe Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to solve that with the SUFFIXES argument. However, I don't know why it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried adding ../bin to the SUFFIXES argument?

if(VAR_GENERATOR_EXPRESSION AND ${CMAKE_VERSION} VERSION_LESS "3.14.0")
include(CreateTempFilePath)
CreateTempFilePath(OUTPUT_VAR file_path)
file(GENERATE OUTPUT "${file_path}" CONTENT "${VAR_CONTENT}")
Copy link
Member

@PhysSong PhysSong Aug 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The counter logic doesn't work because the CMake script is evaluated only once. For the MSVC generator, file(GENERATE) itself is executed once, but the content is evaluated multiple times at the generation time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM, I misunderstood the purpose of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I still wonder: Can we create files with unique names(more readable if possible) without a counter?

cmake_parse_arguments(VAR "${options}" "${oneValueArgs}"
"${multiValueArgs}" ${ARGN} )

# install(CODE) does not support generator expression in ver<3.14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, CMP0087 should be set to use generator expressions in CMake >= 3.14.

@PhysSong
Copy link
Member

Regarding the MSVC failure: I debugged the generation logic and got values from different configurations.
RemoteVstPlugin:

C:/projects/lmms/build/plugins/32/RemoteVstPlugin32.exe
C:/projects/lmms/build/plugins/32/RemoteVstPlugin32.exe
C:/projects/lmms/build/plugins/32/RemoteVstPlugin32.exe
C:/projects/lmms/build/plugins/32/RemoteVstPlugin32.exe

LMMS binary:

C:/projects/lmms/build/Debug/lmms.exe
C:/projects/lmms/build/MinSizeRel/lmms.exe
C:/projects/lmms/build/Release/lmms.exe
C:/projects/lmms/build/RelWithDebInfo/lmms.exe

Other plugins:

C:/projects/lmms/build/plugins/Debug/audiofileprocessor.dll;(omitted)
C:/projects/lmms/build/plugins/MinSizeRel/audiofileprocessor.dll;(omitted)
C:/projects/lmms/build/plugins/Release/audiofileprocessor.dll;(omitted)
C:/projects/lmms/build/plugins/RelWithDebInfo/audiofileprocessor.dll;(omitted)

I think we can generate with $<CONFIG> and read with ${CMAKE_BUILD_TYPE} when using multi-config generators like the MSVC generator.

file(GENERATE OUTPUT "${file_path}" CONTENT "${VAR_CONTENT}")
install(CODE "file(READ ${file_path} \"${VAR_NAME}\")")
else()
cmake_policy(SET CMP<0087> NEW)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cmake_policy(SET CMP<0087> NEW)
cmake_policy(SET CMP0087 NEW)

Also, why did you use spaces instead of tabs in some new files?

@Reflexe Reflexe closed this Aug 22, 2019
@PhysSong
Copy link
Member

Due to the GitHub outage, this PR is moved to #5142.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants