RDKEMW-18087 : Build Meminsight in RDKE with latest 1.1.0 changes

[rosemarybennyy]

No description provided.

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/meta-rdk/839/rdk/components/generic/rdk-oe/meta-rdk

• Commit: 199923a

Report detail: gist'

[Copilot]

Pull request overview

Updates the Yocto recipe and systemd integration for the meminsight devtool to align with upstream v1.1.0, and adds a new path-triggered “upload” sidecar intended to react to a trigger file.

Changes:

• Bump meminsight recipe version/SRCREV to v1.1.0 and add a cjson PACKAGECONFIG toggle.
• Add meminsight-upload.service + meminsight-upload.path and an upload_MemReports.sh helper script; enable the new path unit alongside the existing runner path unit.
• Update meminsight-runner.service stop/cleanup behavior and add restart-on-failure settings.

Reviewed changes

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

Show a summary per file

File	Description
recipes-devtools/meminsight/meminsight_git.bb	Version bump, new units/scripts added to SRC_URI + install + packaging, and new cjson configuration toggles.
recipes-devtools/meminsight/files/upload_MemReports.sh	New shell script executed by the upload sidecar service.
recipes-devtools/meminsight/files/meminsight-upload.service	New oneshot service intended to run the upload script.
recipes-devtools/meminsight/files/meminsight-upload.path	New path unit that triggers the upload service when /tmp/.meminsight_upload exists.
recipes-devtools/meminsight/files/meminsight-runner.service	Runner service cleanup expanded; restart policy added.
initial_meta-rdk_changes_meminsight_RDKEMW_17746_patch_000.diff	New patch artifact file added to the repo (appears unrelated to BitBake build inputs).

---

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (13)

recipes-devtools/meminsight/meminsight_git.bb:46

• Lines 43 and 46 contain trailing whitespace after the variable definitions, which is a style/cleanliness issue and can produce warnings in some linters. Please remove the trailing spaces.

 
SYSLOG-NG_SERVICE_meminsight = "meminsight-runner.service"
SYSLOG-NG_SERVICE_meminsight += " meminsight-upload.service"
 

recipes-devtools/meminsight/meminsight_git.bb:59

• The install path /lib/rdk is hardcoded. Other Yocto recipes typically use ${nonarch_base_libdir}/rdk (or at minimum ${base_libdir}/rdk) so that the path remains correct on multilib or non-standard layouts. The corresponding FILES:${PN} entry on line 96 has the same hardcoded path. Consider using the variable for portability and consistency with other RDK recipes.

    install -d ${D}/lib/rdk
    install -m 0755 ${WORKDIR}/upload_MemReports.sh ${D}/lib/rdk/upload_MemReports.sh

recipes-devtools/meminsight/files/meminsight-upload.service:26

• There is a stray space between ExecStart= and the script path. While systemd tolerates this, it is unusual and inconsistent with the ExecStart= line in meminsight-runner.service. Remove the leading whitespace from the command value.

ExecStart= /lib/rdk/upload_MemReports.sh

recipes-devtools/meminsight/files/meminsight-upload.service:26

• Type=oneshot is intended for short-lived commands that complete and exit. However, upload_MemReports.sh runs an infinite while true loop that only exits when meminsight stops. With Type=oneshot + RemainAfterExit=yes, systemd will wait for the ExecStart command to finish before considering the unit "started", which means the unit will be reported as "activating" for the entire lifetime of meminsight. A long-running supervisor like this should normally use Type=simple (matching the change made to meminsight-runner.service in this same PR).

Type=oneshot
RemainAfterExit=yes
ExecStart= /lib/rdk/upload_MemReports.sh

recipes-devtools/meminsight/files/upload_MemReports.sh:68

• Trailing tab/whitespace character after the closing " of the timestamp assignment. Remove the trailing whitespace.

    timestamp="$(date '+%Y-%m-%d %H:%M:%S')"	

recipes-devtools/meminsight/files/upload_MemReports.sh:230

• Iterating with for f in $(find ...) splits on $IFS (whitespace, including spaces and newlines). If any meminsight report filename ever contains whitespace, this loop (and the analogous loops in build_upload_set at line 217, in upload_cycle at lines 396 and 417, and in get_highest_iteration at line 190) will operate on broken paths and may delete or upload the wrong files. Since meminsight filenames are currently controlled, the risk is low, but the pattern is fragile — prefer find ... -print0 | xargs -0 or while IFS= read -r f.

get_highest_iteration() {
    max_iter=-1
    for f in $(find "$OUTPUT_DIR" -maxdepth 1 -type f \( -name '*.csv' -o -name '*.json' \) 2>/dev/null); do
        bname="$(basename "$f")"
        iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)_.*/\1/p')"
        [ -z "$iter" ] && iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)\..*/\1/p')"
        if [ -n "$iter" ] && [ "$iter" -gt "$max_iter" ]; then
            max_iter="$iter"
        fi
    done
    echo "$max_iter"
}

# Build the set of report files eligible for upload in this cycle.
#
# If /tmp/.meminsight_inprogress exists (active write in progress):
#   All .csv/.json in OUTPUT_DIR EXCEPT files belonging to the highest
#   iteration number (which may still be open for writing).
#
# If /tmp/.meminsight_inprogress is absent (no active write):
#   All .csv/.json in OUTPUT_DIR with no exclusions (full drain).
#
# Returns: newline-separated list of absolute paths, or empty string
build_upload_set() {
    all_files="$(find "$OUTPUT_DIR" -maxdepth 1 -type f \( -name '*.csv' -o -name '*.json' \) 2>/dev/null)"
    [ -z "$all_files" ] && return 0

    if is_capture_in_progress; then
        max_iter="$(get_highest_iteration)"
        for f in $all_files; do
            bname="$(basename "$f")"
            iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)_.*/\1/p')"
            [ -z "$iter" ] && iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)\..*/\1/p')"
            if [ -n "$iter" ] && [ "$iter" -eq "$max_iter" ]; then
                log "Skipping active iter${max_iter}: $(basename "$f")"
                continue
            fi
            echo "$f"
        done
    else
        echo "$all_files"
    fi
}

recipes-devtools/meminsight/files/upload_MemReports.sh:280

• By the time this line runs, the surrounding if block has just established that HTTP_UPLOAD_LINK is empty (line 270), so ${HTTP_UPLOAD_LINK:-...} is always equivalent to its alternative form — the inner ${HTTP_UPLOAD_LINK:-...} is dead/redundant logic and makes the precedence harder to read. Simplify to HTTP_UPLOAD_LINK="${UPLOAD_HTTPLINK_URL:+${UPLOAD_HTTPLINK_URL}/cgi-bin/S3.cgi}". Also note that if /etc/dcm.properties (sourced on line 276) does not export UPLOAD_HTTPLINK_URL, HTTP_UPLOAD_LINK will silently remain empty here.

        [ -z "$LOG_SERVER" ] && LOG_SERVER="${LOG_SERVER_URL:-$LOG_SERVER}"
        [ -z "$HTTP_UPLOAD_LINK" ] && HTTP_UPLOAD_LINK="${HTTP_UPLOAD_LINK:-${UPLOAD_HTTPLINK_URL:+${UPLOAD_HTTPLINK_URL}/cgi-bin/S3.cgi}}"

recipes-devtools/meminsight/files/upload_MemReports.sh:468

• load_configstore is called before the UPLOAD_INTERVAL numeric check on line 465. If the configstore file contains a non-numeric UPLOAD_INTERVAL value (e.g. "foo", an empty string, or even something with surrounding whitespace), the -eq comparison on line 465 will throw an integer expression expected error and the script will continue with the bad value, later passing it to sleep. Consider validating that UPLOAD_INTERVAL matches ^[0-9]+$ after parsing the configstore, and falling back to the default if not.

    if [ "$UPLOAD_INTERVAL" -eq 0 ]; then
        UPLOAD_INTERVAL=900
        log "No upload interval configured; defaulting to 900 seconds."
    fi

recipes-devtools/meminsight/files/upload_MemReports.sh:496

• UPLOAD_ENABLED is loaded from the configstore once at startup. After the configstore-change-detection block (line 488-496) calls load_configstore again, the script does not re-check $UPLOAD_ENABLED. If an operator disables uploads at runtime (sets UPLOAD_ENABLED=0), the loop will keep running upload cycles until meminsight exits. Either re-evaluate UPLOAD_ENABLED after reload (and exit if it became false) or document explicitly that runtime disable is not honored.

        current_hash="$(configstore_hash)"
        if [ -n "$current_hash" ] && [ "$current_hash" != "$last_hash" ]; then
            log "Configstore changed (hash $last_hash → $current_hash); reloading."
            load_configstore
            last_hash="$current_hash"
            # Recompute derived values that depend on OUTPUT_DIR / UPLOAD_INTERVAL.
            [ "$UPLOAD_INTERVAL" -eq 0 ] && UPLOAD_INTERVAL=900
            STAGING_DIR="${OUTPUT_DIR%/}_staging"
            log "Config reloaded: interval=${UPLOAD_INTERVAL}s output=$OUTPUT_DIR staging=$STAGING_DIR"
        fi

recipes-devtools/meminsight/files/upload_MemReports.sh:110

• cleanup_lock is registered with trap ... EXIT INT TERM, but cleanup_upload_trigger and the staging-directory cleanup on lines 503-504 only run when the script reaches the end of main naturally (i.e. when upload_cycle returns 1). If the upload service is killed (SIGTERM from systemd stop, etc.) while sleeping or mid-upload, the .meminsight_upload trigger file and any leftover staging .tgz will remain on disk, and the path unit may immediately re-trigger the service on next boot. Consider extending the trap to also clear the trigger and (optionally) the staging archive on signal exit.

acquire_lock() {
    if ! mkdir "$LOCK_DIR" >/dev/null 2>&1; then
        log "Another upload instance is already running; exiting."
        exit 0
    fi
    trap cleanup_lock EXIT INT TERM
}

recipes-devtools/meminsight/files/upload_MemReports.sh:230

• get_highest_iteration (line 188) and build_upload_set (line 211) each independently run find and sed over the same set of files to extract iteration numbers. These two functions duplicate the same parsing logic. Consider factoring the "extract iter number from basename" snippet into a small helper to avoid drift between the two implementations.

get_highest_iteration() {
    max_iter=-1
    for f in $(find "$OUTPUT_DIR" -maxdepth 1 -type f \( -name '*.csv' -o -name '*.json' \) 2>/dev/null); do
        bname="$(basename "$f")"
        iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)_.*/\1/p')"
        [ -z "$iter" ] && iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)\..*/\1/p')"
        if [ -n "$iter" ] && [ "$iter" -gt "$max_iter" ]; then
            max_iter="$iter"
        fi
    done
    echo "$max_iter"
}

# Build the set of report files eligible for upload in this cycle.
#
# If /tmp/.meminsight_inprogress exists (active write in progress):
#   All .csv/.json in OUTPUT_DIR EXCEPT files belonging to the highest
#   iteration number (which may still be open for writing).
#
# If /tmp/.meminsight_inprogress is absent (no active write):
#   All .csv/.json in OUTPUT_DIR with no exclusions (full drain).
#
# Returns: newline-separated list of absolute paths, or empty string
build_upload_set() {
    all_files="$(find "$OUTPUT_DIR" -maxdepth 1 -type f \( -name '*.csv' -o -name '*.json' \) 2>/dev/null)"
    [ -z "$all_files" ] && return 0

    if is_capture_in_progress; then
        max_iter="$(get_highest_iteration)"
        for f in $all_files; do
            bname="$(basename "$f")"
            iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)_.*/\1/p')"
            [ -z "$iter" ] && iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)\..*/\1/p')"
            if [ -n "$iter" ] && [ "$iter" -eq "$max_iter" ]; then
                log "Skipping active iter${max_iter}: $(basename "$f")"
                continue
            fi
            echo "$f"
        done
    else
        echo "$all_files"
    fi
}

recipes-devtools/meminsight/files/upload_MemReports.sh:226

• If is_capture_in_progress is true but no filename matches the _iter<n> pattern, get_highest_iteration returns -1 and the -eq comparison on line 221 will never match, so all files are uploaded. Conversely, if max_iter is -1, the subsequent loop still echoes files with no iteration number — files that contain _iter (matching the pattern) but happen to have iter value -1 cannot occur, so this is benign. However, the function silently returns the literal string -1 which is then interpolated into the log on line 222 only when there is a match. Worth either documenting this -1 sentinel or guarding the [ "$iter" -eq "$max_iter" ] test with [ "$max_iter" -ge 0 ].

    if is_capture_in_progress; then
        max_iter="$(get_highest_iteration)"
        for f in $all_files; do
            bname="$(basename "$f")"
            iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)_.*/\1/p')"
            [ -z "$iter" ] && iter="$(echo "$bname" | sed -n 's/.*_iter\([0-9][0-9]*\)\..*/\1/p')"
            if [ -n "$iter" ] && [ "$iter" -eq "$max_iter" ]; then
                log "Skipping active iter${max_iter}: $(basename "$f")"
                continue
            fi
            echo "$f"
        done

recipes-devtools/meminsight/files/meminsight-runner.service:29

• The original ExecStop removed /tmp/meminsight.env. The new ExecStop no longer removes that file — instead it is moved to ExecStopPost. This is a subtle behavior change: ExecStop runs while the service is stopping (with the service still considered active), while ExecStopPost runs after the unit transitions to inactive/failed. Confirm that this re-ordering is intentional and that no consumer of /tmp/meminsight.env depends on it being removed before the unit becomes inactive. Additionally, ExecStop now relies on glob-free literal paths and /bin/rm will exit nonzero (silently, due to the - prefix) when either trigger file is absent, which is fine but worth noting.

ExecStop=-/bin/rm /tmp/.meminsight_upload /tmp/.meminsight_inprogress
ExecStopPost=-/bin/rm /tmp/meminsight.env /tmp/.enable_meminsight /nvram/.enable_meminsight

[gomathishankar37]

• Added a few comments, Please check
• Address or resolve Co-Pilot comments as well

CC: @tdeva14 , @Abhinavpv28

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

recipes-devtools/meminsight/files/meminsight-upload.service:26

• ExecStart= /lib/rdk/upload_MemReports.sh has an extra space after =. Please remove it to match the unit-file style used elsewhere in the repo and avoid any potential parsing/escaping surprises.

RemainAfterExit=yes
ExecStart= /lib/rdk/upload_MemReports.sh

recipes-devtools/meminsight/meminsight_git.bb:38

• The cjson PACKAGECONFIG only defines the configure flags; it doesn’t declare any build-time dependency. If --enable-cjson requires the external cjson headers/library, the recipe should add cjson as a dependency via the 3rd field of PACKAGECONFIG[cjson] (common pattern in this repo, e.g. reboot-manager.bb:73 adds breakpad) and/or DEPENDS += "cjson". RDEPENDS alone won’t populate the sysroot for compilation.

PACKAGECONFIG ??= "cjson"
PACKAGECONFIG[cjson] = "--enable-cjson,--disable-cjson"

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

recipes-devtools/meminsight/meminsight_git.bb:44

• SYSLOG-NG_SERVICE_meminsight is being built by appending a second systemd unit into the same variable. The syslog-ng config generator (recipes-core/images/syslog-ng-config.inc:update_filters) treats each SYSLOG-NG_SERVICE_ entry as a single SYSTEMD_UNIT string; a space-separated list will not match either unit, so logs from these services may be dropped. Prefer defining separate SYSLOG-NG_FILTER entries (one per unit) and mapping each filter to exactly one SYSLOG-NG_SERVICE* value (they can still share the same destination file if needed).

SYSLOG-NG_FILTER = "meminsight"
SYSLOG-NG_SERVICE_meminsight = "meminsight-runner.service"
SYSLOG-NG_SERVICE_meminsight += " meminsight-upload.service"
SYSLOG-NG_DESTINATION_meminsight = "meminsight.log"

recipes-devtools/meminsight/meminsight_git.bb:56

• This recipe installs the upload helper under a hardcoded /lib/rdk path (${D}/lib/rdk) instead of using ${base_libdir}/rdk, which is the pattern used elsewhere (e.g., recipes-common/sys_mon_tools/cpuprocanalyzer_git.bb:35-37, recipes-extended/remotedebugger/remotedebugger.bb:54-58). Using ${base_libdir} avoids path inconsistencies across distros/architectures and keeps FILES entries consistent.

    install -d ${D}/lib/rdk
    install -m 0755 ${WORKDIR}/upload_MemReports.sh ${D}/lib/rdk/upload_MemReports.sh

[svc-rdkeportal01]

CCI-Build-Verified +1
sha: 9a6e6b7

[svc-rdkeportal01]

CCI-Build-Verified +1
sha: 9a6e6b7

[svc-rdkeportal01]

CCI-Test-Verified +1
sha: 9a6e6b7

[tdeva14]

LGTM

[svc-rdkeportal01]

CCI-Build-Verified +1
sha: 9a6e6b7

[svc-rdkeportal01]

CCI-Test-Verified +1
sha: 9a6e6b7

[svc-rdkeportal01]

CCI-Build-Verified +1
sha: 9a6e6b7