RDK-59955: Bring in Crashupload compiled code#452
RDK-59955: Bring in Crashupload compiled code#452gomathishankar37 wants to merge 34 commits intodevelopfrom
Conversation
Signed-off-by: P S, Gomathi Shankar <gomathishankar_ps@comcast.com>
There was a problem hiding this comment.
Pull request overview
This pull request updates the crashupload component to include compiled C code instead of relying solely on shell scripts. The changes update dependency SRCREVs and significantly enhance the crashupload BitBake recipe with compilation support, new dependencies, logging configuration, and systemd integration.
Changes:
- Updated SRCREV references for commonutilities and dcmd dependencies to newer commits
- Transformed crashupload recipe to build compiled binaries with comprehensive build configuration including CFLAGS, LDFLAGS, and multiple new dependencies (curl, openssl, rfc, telemetry, etc.)
- Added syslog-ng and logrotate configuration support for crash upload logging
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| recipes-common/utils/commonutilities_git.bb | Updated SRCREV to newer commit hash |
| recipes-common/dcmd/dcmd.bb | Updated SRCREV to newer commit hash |
| recipes-common/crashupload/crashupload_git.bb | Major refactoring to build compiled crashupload binary with new dependencies, build flags, logging configuration, and enhanced systemd integration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export USE_DBUS = "y" | ||
|
|
||
| LDFLAGS += "-Wl,-O1" | ||
| LDFLAGS:append = "-Wl,-O1" |
There was a problem hiding this comment.
Missing space before operator. The LDFLAGS append operation is missing the colon separator between append and the equals sign. This should be LDFLAGS:append = "-Wl,-O1" to match the pattern used elsewhere in the file (like line 20 with CFLAGS:append).
| LDFLAGS:append = "-Wl,-O1" | |
| LDFLAGS:append = " -Wl,-O1" |
| DEPENDS:append:client = " \ | ||
| curl \ | ||
| openssl \ | ||
| zlib \ | ||
| libarchive \ | ||
| libsyswrapper \ | ||
| rdk-logger \ | ||
| commonutilities \ | ||
| rfc \ | ||
| telemetry \ | ||
| " |
There was a problem hiding this comment.
Inconsistent indentation with tabs used for dependencies. The DEPENDS:append:client section uses tabs for indentation while the rest of the file appears to use spaces. This should be consistent with the indentation style used elsewhere in the file.
| install -d ${D}${base_libdir}/rdk | ||
| install -d ${D}${sysconfdir} ${D}${sysconfdir}/rfcdefaults | ||
| install -m 0755 ${S}/uploadDumps.sh ${D}${base_libdir}/rdk | ||
| install -m 0755 ${WORKDIR}/git/runDumpUpload.sh ${D}${base_libdir}/rdk |
There was a problem hiding this comment.
Inconsistent indentation with tabs. Line 68 uses tabs while most other lines in the do_install section use spaces. This should be consistent with the surrounding indentation style.
| install -m 0755 ${WORKDIR}/git/runDumpUpload.sh ${D}${base_libdir}/rdk | |
| install -m 0755 ${WORKDIR}/git/runDumpUpload.sh ${D}${base_libdir}/rdk |
| minidump-on-bootup-upload.timer \ | ||
| " | ||
| RDEPENDS:${PN} += "busybox" | ||
| " |
There was a problem hiding this comment.
Inconsistent indentation with tabs. Line 92 uses tabs while the rest of the SYSTEMD_SERVICE assignment uses spaces. This should be consistent with the surrounding indentation style.
| " | |
| " |
| #-DT2_EVENT_ENABLED \ | ||
| #" |
There was a problem hiding this comment.
The commented-out code on lines 26-27 should either be removed or properly documented with an explanation. Leaving fragmentary commented code without context can be confusing for future maintainers. If T2_EVENT_ENABLED support is intended for future use, add a comment explaining why it's disabled and under what conditions it should be enabled.
| #-DT2_EVENT_ENABLED \ | |
| #" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #-DT2_EVENT_ENABLED \ | ||
| #" |
There was a problem hiding this comment.
Commented out code should be removed. Lines 26-27 contain commented out preprocessor definitions that should either be uncommented if needed, or removed to improve code maintainability.
| #-DT2_EVENT_ENABLED \ | |
| #" |
|
|
||
| LICENSE = "Apache-2.0" | ||
| LIC_FILES_CHKSUM = "file://LICENSE;md5=175792518e4ac015ab6696d16c4f607e" | ||
| LIC_FILES_CHKSUM = "file://../LICENSE;md5=175792518e4ac015ab6696d16c4f607e" |
There was a problem hiding this comment.
License file path changed from "file://LICENSE" to "file://../LICENSE". This suggests the LICENSE file is now one directory level up from the working directory. Verify that this path is correct given that S is now set to "${WORKDIR}/git/c_sourcecode" on line 14, which would make the LICENSE file at "${WORKDIR}/git/LICENSE".
| rfc \ | ||
| telemetry \ | ||
| " | ||
| RDEPENDS:${PN} += "busybox commonutilities telemetry" |
There was a problem hiding this comment.
RDEPENDS placement creates duplication. Line 63 adds RDEPENDS:${PN} with "busybox commonutilities telemetry", but this duplicates the "busybox" dependency that was previously at the end of the file. The consolidated RDEPENDS should be placed in one location to avoid confusion and potential conflicts.
| RDEPENDS:${PN} += "busybox commonutilities telemetry" | |
| RDEPENDS:${PN} += "commonutilities telemetry" |
| LDFLAGS += "-Wl,-O1" | ||
| LDFLAGS:append = "-Wl,-O1" | ||
|
|
||
| LDFLAGS += "-lrfcapi -ltelemetry_msgsender" |
There was a problem hiding this comment.
LDFLAGS uses += operator instead of :append. Line 32 uses LDFLAGS:append while line 34 uses LDFLAGS +=. This inconsistency could lead to different behavior. The :append operator is preferred in modern Yocto/BitBake as it defers evaluation and avoids whitespace issues. Consider using LDFLAGS:append consistently.
| LDFLAGS += "-lrfcapi -ltelemetry_msgsender" | |
| LDFLAGS:append = " -lrfcapi -ltelemetry_msgsender" |
| #HDD_ENABLE | ||
| LOGROTATE_SIZE_crashupload="128000" | ||
| LOGROTATE_ROTATION_crashupload="3" | ||
| #HDD_DISABLE |
There was a problem hiding this comment.
Commented out code should be removed. Lines 45-50 contain commented out configuration blocks (HDD_ENABLE and HDD_DISABLE) that should either be implemented using proper conditional logic if needed, or removed to improve code maintainability.
| #HDD_ENABLE | |
| LOGROTATE_SIZE_crashupload="128000" | |
| LOGROTATE_ROTATION_crashupload="3" | |
| #HDD_DISABLE | |
| LOGROTATE_SIZE_crashupload="128000" | |
| LOGROTATE_ROTATION_crashupload="3" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DEPENDS:append:client = " \ | ||
| curl \ | ||
| openssl \ | ||
| zlib \ | ||
| libarchive \ | ||
| libsyswrapper \ | ||
| rdk-logger \ | ||
| commonutilities \ | ||
| rfc \ | ||
| telemetry \ | ||
| " |
There was a problem hiding this comment.
Inconsistent indentation with tabs and spaces. Lines 53, 55-58 use tabs while lines 54, 56 use spaces. This should be consistent with the rest of the file for maintainability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export USE_DBUS = "y" | ||
|
|
||
| LDFLAGS += "-Wl,-O1" | ||
| LDFLAGS:append = "-Wl,-O1" |
There was a problem hiding this comment.
LDFLAGS:append should include a leading space before the value to prevent concatenation issues. The value should be ' -Wl,-O1' not '-Wl,-O1'.
| LDFLAGS:append = "-Wl,-O1" | |
| LDFLAGS:append = " -Wl,-O1" |
RDK-60393: Memcapture as a downloadable app (#465)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CFLAGS:append = " \ | ||
| -I=${libdir}/glib-2.0/include \ | ||
| -I=${includedir}/glib-2.0 \ | ||
| -DLIBRDKCERTSELECTOR \ |
There was a problem hiding this comment.
The include flags are written as -I=${...} which passes a directory name starting with = to the compiler (e.g., =/usr/lib/...), and is not the normal/valid -I<dir> form. Use -I${libdir}/... (or -I ${libdir}/...) and the same for ${includedir}.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # To have a possibility to override SRC_URI later, we are introducing the following workaround: | ||
| SRCREV = "5c7dfc405cd34da3ab1a1121caf8733ebe35d879" | ||
| SRCREV = "c325e2589e4bd7b78dd5c713b97b23b523746f97" | ||
|
|
||
| SRC_URI = "${CMF_GITHUB_ROOT}/common_utilities;module=.;${CMF_GITHUB_SRC_URI_SUFFIX}" |
There was a problem hiding this comment.
This SRCREV pin is likely being overridden for RDKB builds by conf/distro/include/rdkb.inc, which sets SRCREV:pn-commonutilities (rdkb.inc:199). If this bump is required for the PR’s behavior, please update the distro override as well; otherwise this change may have no effect in the main build configuration.
| # SRCREV = "59925c3570c89347a8e05d852401db923337c495" | ||
|
|
||
| # Topic Work - line with develop | ||
| # SRCREV = "75db8702230bf3100ac790fa50e26cc24d8c5c63" | ||
|
|
||
| # topic/RDK-59955 | ||
| #SRCREV = "4eb66b647e35773da34a61e21c7be98745666dc5" | ||
|
|
||
| # Previous Reboot + Ratelimit |
There was a problem hiding this comment.
The recipe currently carries multiple commented-out SRCREV values and topic notes. This makes it unclear which revision is intended to be built and can lead to accidental pinning to the wrong commit later. Please remove the stale commented SRCREV lines and keep a single authoritative SRCREV (or use a tag/branch override mechanism) with a brief, stable comment explaining why that revision is chosen.
| # SRCREV = "59925c3570c89347a8e05d852401db923337c495" | |
| # Topic Work - line with develop | |
| # SRCREV = "75db8702230bf3100ac790fa50e26cc24d8c5c63" | |
| # topic/RDK-59955 | |
| #SRCREV = "4eb66b647e35773da34a61e21c7be98745666dc5" | |
| # Previous Reboot + Ratelimit | |
| # Pinned to last known good revision including reboot and rate-limit handling |
| LDFLAGS:append = "-Wl,-O1" | ||
| LDFLAGS:append = " -lrfcapi -ltelemetry_msgsender -lRdkCertSelector -lrdkconfig" | ||
|
|
||
| inherit coverity | ||
| inherit systemd | ||
| inherit autotools systemd coverity pkgconfig | ||
|
|
||
| DEPENDS:append:client = " \ | ||
| curl \ | ||
| openssl \ | ||
| zlib \ | ||
| libarchive \ | ||
| libsyswrapper \ | ||
| rdk-logger \ | ||
| commonutilities \ | ||
| rdkcertconfig \ | ||
| rfc \ | ||
| telemetry \ | ||
| " |
There was a problem hiding this comment.
LDFLAGS is unconditionally appending -lrfcapi -ltelemetry_msgsender -lRdkCertSelector -lrdkconfig, but the corresponding providers (rfc, telemetry, rdkcertconfig, etc.) are only added to DEPENDS under the :client override. Since this recipe is also built for :broadband (it has do_install:append:broadband()), this creates a hidden build-time dependency and can cause non-client builds to fail at link time. Either scope these LDFLAGS additions to the same override(s) that build the binary, or move the required providers into the unconditional DEPENDS so all variants have the needed sysroot libraries.
| # Previous Reboot + Ratelimit | ||
| SRCREV = "7a2344091ad2a9e19edd0b73b1da01cc49ee4bfe" | ||
|
|
||
| S = "${WORKDIR}/git/c_sourcecode" |
There was a problem hiding this comment.
This SRCREV pin is likely not taking effect for RDKB builds because conf/distro/include/rdkb.inc already sets SRCREV:pn-crashupload (currently at rdkb.inc:198). If the goal of this PR is to bring in the new crashupload compiled code, that distro override also needs to be updated (or removed) so the build actually fetches this revision.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CFLAGS:append = " \ | ||
| -I=${libdir}/glib-2.0/include \ | ||
| -I=${includedir}/glib-2.0 \ | ||
| -DLIBRDKCERTSELECTOR \ | ||
| -DRFC_API_ENABLED \ | ||
| -DT2_EVENT_ENABLED \ | ||
| -DRDK_LOGGER \ | ||
| -DUSE_EXTENDED_LOGGER_INIT \ | ||
| " |
There was a problem hiding this comment.
The compile-time feature macros (-DRFC_API_ENABLED, -DT2_EVENT_ENABLED, -DRDK_LOGGER, etc.) are applied to all builds via CFLAGS:append, but the dependencies that typically provide these headers/libraries are only appended for the client override. If non-client variants don’t have those dependencies staged, this can break compilation or change behavior unintentionally. Consider scoping these macros to :client (or adding the matching dependencies to the base DEPENDS if they are required for all variants).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CFLAGS:append = " \ | ||
| -I=${libdir}/glib-2.0/include \ | ||
| -I=${includedir}/glib-2.0 \ | ||
| -DLIBRDKCERTSELECTOR \ | ||
| -DRFC_API_ENABLED \ | ||
| -DT2_EVENT_ENABLED \ | ||
| -DRDK_LOGGER \ | ||
| -DUSE_EXTENDED_LOGGER_INIT \ | ||
| " | ||
|
|
||
| export GLIBS = "-lglib-2.0 -lz" | ||
| export USE_DBUS = "y" | ||
|
|
||
| LDFLAGS += "-Wl,-O1" | ||
| LDFLAGS:append = "-Wl,-O1" | ||
| LDFLAGS:append = " -lrfcapi -ltelemetry_msgsender -lRdkCertSelector -lrdkconfig" | ||
|
|
There was a problem hiding this comment.
CFLAGS:append and LDFLAGS:append enable RFC/Telemetry/RdkCertSelector features and link against -lrfcapi -ltelemetry_msgsender -lRdkCertSelector -lrdkconfig unconditionally, but the corresponding build dependencies are only added under DEPENDS:append:client. Since the recipe now inherits autotools, the compile/link steps will run for all overrides (including broadband) and can fail due to missing headers/libs in the sysroot. Either (a) move these feature defines and link flags under :client, or (b) add the required DEPENDS for all variants, or (c) explicitly disable the autotools build tasks for non-client variants if they only need the scripts/services.
|
|
||
| LDFLAGS:append = " -lsafec -lsecure_wrapper" | ||
|
|
There was a problem hiding this comment.
LDFLAGS:append unconditionally adds -lsafec -lsecure_wrapper, but the recipe otherwise treats safec as optional (adds -DSAFEC_DUMMY_API and conditionally appends libs via pkg-config). This unconditional link will break builds when safec is not in DISTRO_FEATURES, and it also introduces a link-time dependency on secure_wrapper without adding the corresponding DEPENDS (typically libsyswrapper). Make linking against safec/secure_wrapper conditional (or rely solely on the existing conditional pkg-config LDFLAGS) and add the correct provider to DEPENDS if secure_wrapper is required.
| LDFLAGS:append = " -lsafec -lsecure_wrapper" | |
| DEPENDS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', ' libsyswrapper', '', d)}" | |
| LDFLAGS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', ' -lsafec -lsecure_wrapper', '', d)}" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SYSTEMD_SERVICE:${PN} += "dcmd.service" | ||
| inherit breakpad-wrapper | ||
| DEPENDS += "breakpad breakpad-wrapper" | ||
| BREAKPAD_BIN:append = "logupload" |
There was a problem hiding this comment.
:append does not automatically add whitespace; without a leading space this will concatenate onto any existing value (e.g., "...existing"logupload). Use a leading space in the appended string (or +=) so logupload becomes a separate token.
| BREAKPAD_BIN:append = "logupload" | |
| BREAKPAD_BIN:append = " logupload" |
|
|
||
|
|
||
| # Add any extra packaging if needed | ||
| FILES_${PN} += "${bindir}/logupload" |
There was a problem hiding this comment.
This uses the older underscore override syntax (FILES_${PN}) while the rest of the recipe uses the newer override style (SYSTEMD_SERVICE:${PN}). For consistency and to avoid override edge-cases on newer BitBake, prefer FILES:${PN} += "${bindir}/logupload".
| FILES_${PN} += "${bindir}/logupload" | |
| FILES:${PN} += "${bindir}/logupload" |
| export USE_DBUS = "y" | ||
|
|
||
| LDFLAGS += "-Wl,-O1" | ||
| LDFLAGS:append = "-Wl,-O1" |
There was a problem hiding this comment.
LDFLAGS:append should include a leading space; otherwise the appended value can get glued to the previous token in LDFLAGS, producing an invalid linker argument string. Change to LDFLAGS:append = " -Wl,-O1".
| LDFLAGS:append = "-Wl,-O1" | |
| LDFLAGS:append = " -Wl,-O1" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # or enable this distro feature | ||
| CFLAGS += "${@bb.utils.contains('DISTRO_FEATURES', 'debug_curl_cdl', ' -DCURL_DEBUG', '', d)}" | ||
|
|
||
| DEPENDS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', ' safec', " ", d)}" |
There was a problem hiding this comment.
This bb.utils.contains() call has unescaped double quotes around the fallback value (" "), which will prematurely terminate the BitBake string and break parsing. Use single quotes for the fallback (e.g., '' or ' ') or otherwise escape the inner quotes.
| DEPENDS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', ' safec', " ", d)}" | |
| DEPENDS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', ' safec', ' ', d)}" |
| CFLAGS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', '', ' -DSAFEC_DUMMY_API', d)}" | ||
| LDFLAGS:append = " ${@bb.utils.contains('DISTRO_FEATURES', 'safec', ' `pkg-config --libs libsafec`', '', d)}" | ||
|
|
||
| LDFLAGS:append = " -lsafec -lsecure_wrapper" |
There was a problem hiding this comment.
LDFLAGS unconditionally appends -lsafec even when DISTRO_FEATURES does not include 'safec' (and the recipe explicitly supports a SAFEC_DUMMY_API path). This can cause link failures on builds without libsafec; make this link conditional (or ensure libsafec is always provided via DEPENDS in all builds).
| LDFLAGS:append = " -lsafec -lsecure_wrapper" | |
| LDFLAGS:append = " -lsecure_wrapper" |
No description provided.