allow make install to work for module by ron003 · Pull Request #72 · art-daq/trace

ron003 · 2026-07-21T21:50:27Z

after cmake -DWANT_KMOD=ON -DCMAKE_INSTALL_PREFIX=/ ..
make install should install utilities and module in expected locations.

Copilot

Pull request overview

This PR updates the CMake build/install logic so that when the TRACE kernel module is enabled, make install places the built TRACE.ko into a kernel-module directory and triggers module dependency refresh.

Changes:

Adds a top-level WANT_KMOD CMake option to control whether the kernel module is built.
Changes kernel module installation to install TRACE.ko into /lib/modules/<kver>/... and runs depmod after install.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`CMakeLists.txt`	Introduces the `WANT_KMOD` option so module build logic can be toggled from the top-level configuration.
`src_module/CMakeLists.txt`	Installs the built kernel module to a module directory and invokes `depmod` at install time.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src_module/CMakeLists.txt:29

The install step currently hard-codes /sbin/depmod and runs depmod -a unconditionally. This can unintentionally modify the host module dependency files during staged installs (e.g., when DESTDIR is set) and may fail on systems where depmod isn’t in /sbin. Consider locating depmod via find_program and only running it for real system installs (e.g., CMAKE_INSTALL_PREFIX is / and DESTDIR is not set). Also, the cache help text mentions /lib/modules/... but the destination is relative to CMAKE_INSTALL_PREFIX.

        set(TRACE_KMOD_INSTALL_DIR "extra/TRACE" CACHE STRING "Subdir under /lib/modules/<kver>/ for TRACE.ko")
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/../module/${KVERS}/TRACE.ko
  DESTINATION "lib/modules/${KVERS}/${TRACE_KMOD_INSTALL_DIR}")
        install(CODE "execute_process(COMMAND /sbin/depmod -a ${KVERS})")

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src_module/CMakeLists.txt:38

depmod -a is executed on every non-DESTDIR install, even when installing the module under a non-system prefix (e.g., CMAKE_INSTALL_PREFIX=/opt/...). That can unexpectedly modify the host system’s module dependency files under /lib/modules even though the module wasn’t installed there. Gate the depmod step to system installs (prefix / or /usr) in addition to the existing DESTDIR check.

    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\")\n
    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

 install(PROGRAMS bitN_to_mask trace_envvars trace_delta trace-cmd-merge trace_color trace-cmd-merge trace_bash trace_bash_example.sh DESTINATION bin)
-install(PROGRAMS trace_functions.sh DESTINATION etc/profile.d)
+install(PROGRAMS trace_functions.sh DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/profile.d)
 install(PROGRAMS trace_functions.sh DESTINATION bin)  # OK of DUNE patch (to this file) causes double bin install


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src_module/CMakeLists.txt:38

depmod -a is currently run whenever DESTDIR is empty, even for non-system prefixes where the module is installed under <prefix>/lib/modules/.... In that case, running depmod will operate on the host’s /lib/modules tree (and may fail or unexpectedly modify the system), which can break make install for local-prefix installs. Gate the depmod step on system prefixes (e.g., / and /usr), and consider capturing failures so they don’t silently pass.

    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/../module/${KVERS}/TRACE.ko
	        DESTINATION "${TRACE_KMOD_INSTALL_DEST}")
    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\")\n
    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

Before the change: ``` $ rm -fr ../build/*; cmake -DWANT_KMOD=ON -DCMAKE_INSTALL_PREFIX=/ .. 2>/dev/null | grep 'source tree' -- TRACE source tree: install=ON, dir=share/TRACE, effective=/share/TRACE ``` Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

script/CMakeLists.txt:30

Using CMAKE_INSTALL_FULL_SYSCONFDIR makes this install destination absolute (prefix baked in at configure time), which breaks cmake --install ... --prefix <new> relocatable installs and is inconsistent with how other install destinations are specified. Use the relative CMAKE_INSTALL_SYSCONFDIR here so the effective prefix can still be overridden at install time.

install(PROGRAMS trace_functions.sh DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/profile.d)

src_module/CMakeLists.txt:38

Running depmod unconditionally (when DESTDIR is empty) can fail the install if depmod is not on PATH (often under /sbin//usr/sbin) or if it returns a non-zero status. Make this step best-effort by locating depmod and warning (not failing) on errors so make install remains robust.

    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\")\n
    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

  install(FILES ${CMAKE_CURRENT_BINARY_DIR}/etc/compiler_info
-    DESTINATION ${CMAKE_INSTALL_SYSCONFDIR})
+    DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR})


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src_module/CMakeLists.txt:38

The depmod install step runs whenever DESTDIR is empty, even for non-system prefixes where the module is installed under <prefix>/lib/modules/.... In that case depmod -a ${KVERS} will update the host system’s /lib/modules instead of the install tree (and may fail without root), which is incorrect for prefix installs.

    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/../module/${KVERS}/TRACE.ko
	        DESTINATION "${TRACE_KMOD_INSTALL_DEST}")
    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\")\n
    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

ron003 · 2026-07-24T03:02:23Z

I did test this in the dbt-create -s environment, and it (including python) seems to be OK.

ron003 · 2026-07-24T03:59:10Z

Wait - there seems to be a problem with PYTHONPATH

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

script/CMakeLists.txt:30

Using ${CMAKE_INSTALL_FULL_SYSCONFDIR} here installs to /usr/etc/profile.d when CMAKE_INSTALL_PREFIX=/usr, but the README and typical system integration expect /etc/profile.d. This makes the system install layout inconsistent depending on whether the prefix is / or /usr.

install(PROGRAMS trace_functions.sh DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/profile.d)

src_module/CMakeLists.txt:38

The install-time depmod invocation can cause make install to fail (e.g., depmod not in PATH, insufficient privileges, or depmod returning non-zero). Since this runs as part of install, it's safer to locate depmod and treat failures as warnings so installation of TRACE.ko still succeeds.

    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\" AND (\"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/\" OR \"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/usr\"))\n
    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

More robust, but more involved/complex Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

script/CMakeLists.txt:30

When configuring with -DCMAKE_INSTALL_PREFIX=/usr, GNUInstallDirs typically sets CMAKE_INSTALL_FULL_SYSCONFDIR to /usr/etc, so this installs trace_functions.sh under /usr/etc/profile.d rather than the expected system-wide /etc/profile.d. That makes the "system install" behavior differ between prefix=/ and prefix=/usr and can break system shell integration.

install(PROGRAMS trace_functions.sh DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/profile.d)

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

script/CMakeLists.txt:31

For CMAKE_INSTALL_PREFIX=/usr, GNUInstallDirs typically sets CMAKE_INSTALL_FULL_SYSCONFDIR to /usr/etc (since SYSCONFDIR defaults to "etc"), so this will install to /usr/etc/profile.d rather than the intended /etc/profile.d for system integration (also described in README.md). Consider special-casing system prefixes (/ and /usr) to install directly under /etc, while keeping prefix-relative installs for non-system prefixes.

install(PROGRAMS trace_functions.sh DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/profile.d)
install(PROGRAMS trace_functions.sh DESTINATION bin)  # OK if DUNE patch (to this file) causes double bin install

+    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\" AND (\"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/\" OR \"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/usr\"))\n
+    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")


README.md Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src_module/CMakeLists.txt:38

depmod is executed during installation but its exit status is ignored, so failures (missing depmod, insufficient permissions, etc.) will be silent and can leave the module unregistered even though make install succeeds. Capture the result and emit a warning (or make it fatal if desired) so operators can detect install-time issues.

    install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\" AND (\"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/\" OR \"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/usr\"))\n
    execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

script/CMakeLists.txt:30

Using CMAKE_INSTALL_FULL_SYSCONFDIR hard-codes an absolute destination computed at configure time, which can break cmake --install --prefix … and is inconsistent with the other install rules that use relative destinations. Using CMAKE_INSTALL_SYSCONFDIR still supports both relative (etc) and absolute (/etc) sysconf dirs while preserving install-time prefix overrides.

install(PROGRAMS trace_functions.sh DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/profile.d)

CMakeLists.txt:229

Installing to CMAKE_INSTALL_FULL_SYSCONFDIR bakes in the configure-time prefix. Using CMAKE_INSTALL_SYSCONFDIR keeps the destination relative (or absolute if the user sets it to /etc) and continues to work with install-time prefix overrides (cmake --install --prefix …).

  install(FILES ${CMAKE_CURRENT_BINARY_DIR}/etc/compiler_info
    DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR})

allow make install to work for module

8b07759

Copilot AI review requested due to automatic review settings July 21, 2026 21:50

art-daq-bot added this to art-daq Work Tracker Jul 21, 2026

github-project-automation Bot moved this to 📋 Triage in art-daq Work Tracker Jul 21, 2026

Copilot started reviewing on behalf of ron003 July 21, 2026 21:50 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Comment thread src_module/CMakeLists.txt Outdated

Comment thread src_module/CMakeLists.txt Outdated

ron003 and others added 2 commits July 22, 2026 11:43

Merge branch 'develop' into ron/module_cleanup

efb50e9

Potential fix for pull request finding

f032952

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 22, 2026 16:45

Copilot started reviewing on behalf of ron003 July 22, 2026 16:46 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

rework install -- should not effect non-system installs

0d71f8b

Copilot AI review requested due to automatic review settings July 23, 2026 15:04

Copilot started reviewing on behalf of ron003 July 23, 2026 15:04 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

typo

b3ca665

Copilot AI review requested due to automatic review settings July 23, 2026 21:26

Copilot started reviewing on behalf of ron003 July 23, 2026 21:26 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread CMakeLists.txt Outdated

Copilot AI review requested due to automatic review settings July 23, 2026 21:46

Copilot started reviewing on behalf of ron003 July 23, 2026 21:46 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread CMakeLists.txt

Comment on lines 201 to +202

install(FILES ${CMAKE_CURRENT_BINARY_DIR}/etc/compiler_info

DESTINATION ${CMAKE_INSTALL_SYSCONFDIR})

DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR})

fixed bug (with="/") in last commit

7c9d2a8

Copilot AI review requested due to automatic review settings July 23, 2026 23:46

Copilot started reviewing on behalf of ron003 July 23, 2026 23:46 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

ron003 requested a review from eflumerf July 24, 2026 03:00

backward compat python install

6023e7e

Copilot AI review requested due to automatic review settings July 24, 2026 04:46

Copilot started reviewing on behalf of ron003 July 24, 2026 04:46 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread CMakeLists.txt Outdated

Potential fix for pull request finding

0a8f506

More robust, but more involved/complex Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 24, 2026 04:57

Copilot started reviewing on behalf of ron003 July 24, 2026 04:57 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread README.md

fix make install escaping bug

5adc741

Copilot AI review requested due to automatic review settings July 24, 2026 05:05

Copilot started reviewing on behalf of ron003 July 24, 2026 05:06 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src_module/CMakeLists.txt

Comment on lines +37 to +38

install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\" AND (\"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/\" OR \"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/usr\"))\n

execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

Potential fix for pull request finding

3187abd

README.md Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 24, 2026 05:10

Copilot started reviewing on behalf of ron003 July 24, 2026 05:10 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

		install(CODE "if(\"\$ENV{DESTDIR}\" STREQUAL \"\" AND (\"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/\" OR \"${CMAKE_INSTALL_PREFIX}\" STREQUAL \"/usr\"))\n
		execute_process(COMMAND depmod -a \"${KVERS}\")\nendif()")

Uh oh!

Conversation

ron003 commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ron003 commented Jul 24, 2026

Uh oh!

ron003 commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants