Skip to content

fix: desktop editors with new msvc build routine - #96

Closed
rikled wants to merge 5 commits into
mainfrom
fix/msvcdesktopprep
Closed

fix: desktop editors with new msvc build routine#96
rikled wants to merge 5 commits into
mainfrom
fix/msvcdesktopprep

Conversation

@rikled

@rikled rikled commented Jun 11, 2026

Copy link
Copy Markdown
Member
  • adds cef, qt, icu-desktop as third party dependencies with build scripts.
  • adds a dockerfile to build qt, cef on old ubuntu 18.04
  • fixes small bug in OfficeUtils

rikled added 2 commits June 11, 2026 14:54
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
@rikled
rikled marked this pull request as ready for review June 11, 2026 23:09
@rikled
rikled requested a review from a team as a code owner June 11, 2026 23:09
@rikled
rikled requested review from juliusknorr and removed request for a team June 11, 2026 23:09
@juliusknorr
juliusknorr requested review from a team, Aiiaiiio and chrip and removed request for a team June 16, 2026 20:50

@chrip chrip left a comment

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.

Summary

This PR introduces automated build scripts for three desktop dependencies (CEF branch 5414, Qt 5.9.9, ICU 60.3) plus a Ubuntu 18.04 Dockerfile to build them, following the existing build_3rdparty.py pattern. A minor CMake fix in OfficeUtils is included.

Blockers (Must Fix Before Merge)

🔴 Critical Security Issue - Credential Leak in Dockerfile

File: .docker/third-party.bake.Dockerfile (lines 14-15)

ARG NEXTCLOUD_USER
ARG NEXTCLOUD_PASS
ENV NEXTCLOUD_USER=${NEXTCLOUD_USER}
ENV NEXTCLOUD_PASS=${NEXTCLOUD_PASS}

Problem: These environment variables persist in every image layer and are visible via docker inspect, creating a credential leak that violates the AI Contribution Policy's security requirements.
Fix: Use BuildKit secrets (--secret) or consume the ARG directly in the RUN step that needs it—never promote to ENV.

🔴 Runtime Error - Missing nc. Prefix

File: Common/3dParty/icu-desktop/nc-build.py (line ~1003)
Problem: abort_op(f"Unkown target platform: {sys.platform}") should be nc.abort_op(...) — causes NameError at runtime. Same typo ("Unkown") appears in qt/nc-build.py.
Fix: Add nc. prefix and correct spelling to "Unknown".

🔴 Bash Syntax Error in ICU iOS Script

File: Common/3dParty/icu-desktop/icu_ios.sh (lines 651-654)
Problem: IOS_MIN_VER = "-miphoneos-version-min=11.0" — spaces around = break Bash assignment.
Fix: IOS_MIN_VER="-miphoneos-version-min=11.0"

High-Priority Issues (Should Fix)

🟠 Supply-Chain Risk - CEF Script from Master Branch

File: Common/3dParty/cef/nc-build.py
Problem: automate_git_url points to master branch while cef_checkout is pinned to a specific commit.
Risk: Non-reproducible builds if master changes, violating quality expectations.
Fix: Fetch the automate script from the same tag/ref as the CEF checkout.

🟠 ICU Version Mismatch

Problem: Build scripts and common.cmake reference ICU 60.3, but icu.pri sets ICU_MAJOR_VER = 74.
Risk: Link-time failure when .pri is used (looks for libicuuc.so.74 but finds libicuuc.so.60).
Fix: Align all ICU version references to one consistent version.

🟠 Infinite Recovery Loop Potential

File: Common/3dParty/cef/nc-build.py (lines 267-277)
Problem: After max_recovery_tries, recovery_count resets to 0, allowing infinite retry loop on persistent failure.
Fix: Add a global attempt cap or overall timeout.

🟠 External Script Without Integrity Check

File: Dockerfile (lines 88-90)
Problem: install-build-deps.sh fetched via curl | base64 -d and executed without verification.
Fix: Add sha256sum verification to guard against MITM or CDN compromise.

Recommended Improvements

🟢 Remove iOS/Mobile Scripts from Desktop PR

Files: fetch_mobile.sh, icu_ios.sh
Issue: These are iOS/mobile build scripts placed in icu-desktop/—outside the PR’s stated scope.
Action: Either move to a dedicated mobile PR or relocate to appropriate directory.

🟢 Clean Up Dead Code and Debug Artifacts

  • Dockerfile: Remove or document commented-out packages (e.g., #libxkbcommon-dev, #libssl-dev).
  • nc-build.bat: Remove debug echoes (echo "------------- DBG 1").
  • All new files: Ensure trailing newline (POSIX requirement).

🟢 Clarify Silent CMake Changes

File: OfficeUtils/CMakeLists.txt
Action: Comment why miniunz.c and minizip.c were commented out (e.g., link conflicts) to prevent future confusion.

🟢 Minor Consistency Fixes

  • Standardize make -j$(nproc) vs. hardcoded -j4.
  • Unify ICU/Dockerfile apt-get update runs to reduce image layers.
  • Fix Dockerfile indentation to match repo norms.

What’s Done Well

  • ✅ Follows existing build_3rdparty.py integration pattern.
  • ✅ Adding -u (unbuffered) to Python subprocess improves CI log readability.
  • ✅ Cygwin symlink-dereferencing in nc-build-cygwin.sh is well-commented and addresses a real Windows/WSL hazard.
  • BUILD_DESKTOP guard in common.cmake cleanly gates dependencies—non-desktop builds unaffected.
  • ✅ CEF retry/recovery mechanism (gclient sync → full wipe) is thoughtful.

Assisted-by: OpenClaw:nemotron-3-super

rikled added 2 commits June 17, 2026 12:05
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
@rikled

rikled commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

critical:

  1. fixed security issue -> thanks for that!
  2. fixed abort_op
  3. icu ios irrelevant, as we dont use it yet

high prio:

  1. in the next PR we will pull cef from spotify which fixes this
  2. this is intended, project uses two different icu versions
  3. again spotify fix

@rikled
rikled requested a review from chrip June 17, 2026 19:35
@rikled

rikled commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

close in favor of: #107

@rikled rikled closed this Jun 19, 2026
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