Skip to content

CMake: Alternative pull to add support for a "cmake --install" step. - #1451

Open
linuxdude42 wants to merge 1 commit into
MythTV:masterfrom
linuxdude42:cmake26
Open

CMake: Alternative pull to add support for a "cmake --install" step.#1451
linuxdude42 wants to merge 1 commit into
MythTV:masterfrom
linuxdude42:cmake26

Conversation

@linuxdude42

Copy link
Copy Markdown
Contributor

Add support for a staging directory to hold the results of the "build" stage, and add support for "cmake --install" command to copy the staging directory to the requested install directory. The staging directory is enabled by default for unixish builds, and is ignored for android/windows builds.

Add support for an intermediate install directory to hold the output
of the "build" stage, and then copy this directory to the requested
install directory as the "install" stage.
Comment thread CMakeLists.txt
# Install mythtv. This does nothing more than copy everything from
# the temporary install directory to CMAKE_INSTALL_PREFIX.
if(MYTH_STAGING_PREFIX)
install(DIRECTORY ${MYTH_STAGING_PREFIX}/ DESTINATION . USE_SOURCE_PERMISSIONS)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to exclude the pkg-config files, since they are only relevant during the build. This still creates an empty pkgconfig folder, though:

  install(DIRECTORY ${MYTH_STAGING_PREFIX}/ DESTINATION . USE_SOURCE_PERMISSIONS
          PATTERN "ffnvcodec.pc" EXCLUDE # these headers are not installed
          PATTERN "*.pc" EXCLUDE # these point inside MYTH_STAGING_PREFIX
          )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given the second pattern, the first pattern it redundant, isn't it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, yes, the first pattern is redundant, but the reasoning is different. The comment could probably be: ffnvcodec is not installed and the other pkg-config files point inside MYTH_STAGING_PREFIX.

@ulmus-scott

Copy link
Copy Markdown
Contributor

I was trying to keep the behavior unchanged in #1449, but I think the semantics of CMAKE_INSTALL_PREFIX are better in yours.

Based on how this works with CMAKE_INSTALL_PREFIX, does this obviate a need for a separate MYTH_RUN_PREFIX option?

@linuxdude42

Copy link
Copy Markdown
Contributor Author

Probably? It appears that its only remaining use is (i think) in building the python package. I'm not a python person. I can leave it or remove it, doesn't matter to me. Maybe leave for now and remove in a future commit after checking with someone fluent in python.

@ulmus-scott

Copy link
Copy Markdown
Contributor

I think we should leave MYTH_RUN_PREFIX as is for now. I don't think it would need to be an option anymore, but I'm not sure the super-project's CMAKE_INSTALL_PREFIX is passed through to the sub-projects otherwise. The python bindings only need it to find the grabber scripts and the mythtv executables (data_mythtv.py, to run --version).

git grep -nw INSTALL_PREFIX
mythplugins/mytharchive/README:154:    INSTALL_PREFIX/share/mythtv/ or /etc/mythtv/
mythplugins/mytharchive/README:158:    INSTALL_PREFIX/share/mythtv/mysql.txt
mythtv/bindings/python/MythTV/_versions.py.in:16:INSTALL_PREFIX = '@MYTHTV_INSTALL_PREFIX@'
mythtv/bindings/python/MythTV/dataheap.py:1314:                        prefix=os.path.join(INSTALL_PREFIX, 'share/mythtv'))
mythtv/bindings/python/MythTV/dataheap.py:1342:            long_path = os.path.join(INSTALL_PREFIX, path)
mythtv/programs/scripts/hardwareprofile/distros/mythtv_data/data_mythtv.py:54:            execpath = os.path.join(MythTV.static.INSTALL_PREFIX,

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants