Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modifies a license header comment and updates the CI/CD workflow configuration. However, the license header change introduces a spelling error rather than fixing anything.
Changes:
- Introduces a typo in the Apache license header by splitting "under" into "un der"
- Updates GitHub Actions workflow to use ubuntu-22.04 runner directly instead of a Docker container
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/plat/gdial-plat-util.c | Incorrectly modifies the Apache license header, introducing a spelling error |
| .github/workflows/native_full_build.yml | Updates CI workflow from containerized build to direct ubuntu-22.04 runner |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * limitations un der the License. |
There was a problem hiding this comment.
The word "under" has been incorrectly split into "un der" with an extra space. This should be corrected to maintain the proper Apache license text.
| * limitations un der the License. | |
| * limitations under the License. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 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 2 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.
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * limitations un der the License. |
There was a problem hiding this comment.
This change introduces a typo in the Apache License 2.0 header. The text "limitations un der" has an incorrect space splitting "under" into two words. The correct text should be "limitations under the License" as it was before this change.
| * limitations un der the License. | |
| * limitations under the License. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 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.
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * limitations un der the License. |
There was a problem hiding this comment.
Incorrect spacing in the word 'under'. Should be 'under' not 'un der'.
| * limitations un der the License. | |
| * limitations under the License. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 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.
build_dependencies.sh
Outdated
| -I"$GITHUB_WORKSPACE/stubs" \ | ||
| -I"$INSTALL_DIR/include" \ | ||
| -I"$IARMBUS_PATH/core" \ | ||
| -I"$IARMBUS_PATH/core/include" \ |
There was a problem hiding this comment.
The glib include paths have been removed from the g++ compilation command for libIARMBus.so. The previous version included "-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include" which are no longer present. If the IARM headers (libIBus.h or libIARMCore.h) depend on glib types or headers, this compilation will fail. Consider adding these include paths back or using pkg-config to get the proper glib include flags.
| -I"$IARMBUS_PATH/core/include" \ | |
| -I"$IARMBUS_PATH/core/include" \ | |
| $(pkg-config --cflags glib-2.0) \ |
build_dependencies.sh
Outdated
| # Build trower-base64 | ||
| if [ ! -d "trower-base64" ]; then | ||
| git clone https://github.com/xmidt-org/trower-base64.git | ||
| git clone https://github.com/xmidt-org/trower-base64.git |
There was a problem hiding this comment.
This git clone of https://github.com/xmidt-org/trower-base64.git pulls and builds third-party code from a mutable branch/tag without pinning to an immutable commit or verifying integrity. If the upstream repository or branch is compromised, an attacker-controlled meson.build or source file could execute arbitrary code during meson setup/ninja in your CI or developer environment, potentially with access to secrets and build artifacts. Pin this dependency to a specific commit (or verified release tarball) and, where possible, perform integrity verification before building.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ninja-build meson cmake curl \ | ||
| libsoup2.4-dev libxml2-dev libglib2.0-dev \ | ||
| gobject-introspection libgirepository1.0-dev \ | ||
| libgtk-3-dev libcunit1-dev valac pandoc |
There was a problem hiding this comment.
The package libcunit1-dev is newly added but was not present in the original dependency list. If this is an intentional new dependency, ensure it is actually required for the build process. If not required, it should be removed to avoid unnecessary bloat.
| libgtk-3-dev libcunit1-dev valac pandoc | |
| libgtk-3-dev valac pandoc |
build_dependencies.sh
Outdated
| fi | ||
| ############################## | ||
| # Build trower-base64 | ||
| git clone https://github.com/xmidt-org/trower-base64.git || true |
There was a problem hiding this comment.
Using || true masks clone failures. If the directory already exists, the subsequent cd trower-base64 will succeed but use potentially stale code. Consider checking if the directory exists first, or use git clone with proper error handling to ensure fresh dependencies.
build_dependencies.sh
Outdated
| git clone https://github.com/rdkcentral/iarmbus.git || true | ||
| export IARMBUS_PATH="$GITHUB_WORKSPACE/iarmbus" | ||
|
|
||
| git clone --branch gssdp-1.2.3 https://gitlab.gnome.org/GNOME/gssdp.git | ||
| git clone --branch R4.4.3 https://github.com/rdkcentral/ThunderTools.git || true | ||
| git clone --branch R4.4.1 https://github.com/rdkcentral/Thunder.git || true | ||
| git clone --branch main https://github.com/rdkcentral/entservices-apis.git || true | ||
| git clone https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git || true | ||
| git clone --branch gssdp-1.2.3 https://gitlab.gnome.org/GNOME/gssdp.git || true |
There was a problem hiding this comment.
All git clone commands use || true which silently ignores failures. This can lead to builds using stale or missing dependencies if clones fail. The original code explicitly removed these directories before cloning. Consider either removing existing directories before cloning or checking for successful clones to ensure build reliability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 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 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # System dependencies (sudo REQUIRED in GitHub Actions) | ||
| sudo apt update | ||
| sudo apt install -y \ | ||
| ninja-build meson cmake curl libcurl4-openssl-dev \ |
There was a problem hiding this comment.
The package 'cmake' is now installed via apt, but this is redundant since the script immediately uses cmake commands. Consider verifying that cmake is available or documenting the minimum required version, as ubuntu-22.04 may have an older cmake version than what was in the previous Docker image.
| -S "$GITHUB_WORKSPACE/server" \ | ||
| -B build/xdialserver \ | ||
| -DCMAKE_INSTALL_PREFIX="$INSTALL_PREFIX" \ | ||
| -DCMAKE_MODULE_PATH="$GITHUB_WORKSPACE/install/tools/cmake" \ |
There was a problem hiding this comment.
The CMAKE_MODULE_PATH references '$GITHUB_WORKSPACE/install/tools/cmake' but this directory is created by the ThunderTools build in build_dependencies.sh. There's an implicit dependency on build_dependencies.sh being run first, which should be documented or validated in the script.
No description provided.