Skip to content

Conversation

@AenBleidd
Copy link
Member

@AenBleidd AenBleidd commented Nov 27, 2025

Summary by cubic

Update the x86-linux job to bump apt mirror priority, enable Ubuntu repositories (main, universe, restricted, multiverse), add the Ubuntu Toolchain PPA, add Debian bookworm main, and install g++-i686-linux-gnu along with g++-multilib and gcc-multilib to enforce 32-bit builds during vcpkg test configuration. Also clone vcpkg from AenBleidd/vcpkg and checkout vko_fix_x86-linux_build to test the fix and consistently reproduce the vcpkg test failure.

Written for commit dfb9c7e. Summary will update automatically on new commits.

Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings November 27, 2025 08:50
Copilot finished reviewing on behalf of AenBleidd November 27, 2025 08:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR pins vcpkg to a specific commit (9aee6e968f51e15ee93606f064691d8f6d228190) in response to a test failure. The change adds a git checkout command to lock vcpkg to a known working version for the Linux build job.

Key Changes:

  • Adds vcpkg commit pinning to the Linux job's vcpkg setup step

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

- name: Set up vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vcpkg commit is pinned only for the Linux job. This creates inconsistency across different build jobs (android, macos, mingw, windows), which will use different versions of vcpkg. If this commit is needed to fix a test failure, it should be applied to all jobs that set up vcpkg (lines 86-89, 330-333, 388-391, and 447-450) to ensure consistent behavior across all platforms.

Suggested change
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190
git -C vcpkg checkout 9aee6e968f51e15ee93606f064691d8f6d228190

Copilot uses AI. Check for mistakes.
Signed-off-by: Vitalii Koshura <[email protected]>
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings November 27, 2025 09:19
Copilot finished reviewing on behalf of AenBleidd November 27, 2025 09:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

- name: Set up vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git -C vcpkg checkout c70c929e4854f1add688dab6abd5e0f466864767
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vcpkg revision is only pinned in the Linux job, but other jobs (android, macos, mingw, windows) still clone vcpkg without pinning to this specific revision. This creates inconsistency where different jobs may use different vcpkg versions. If the goal is to reproduce a test failure or ensure deterministic builds, all jobs should pin to the same vcpkg revision.

Copilot uses AI. Check for mistakes.
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings November 27, 2025 10:05
Copilot finished reviewing on behalf of AenBleidd November 27, 2025 10:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

run: |
sudo apt-get update
sudo apt-get install -y g++-multilib gcc-multilib
sudo apt-get install -y crossbuild-essential-i386
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package crossbuild-essential-i386 is designed for cross-compilation from amd64 to i386, but the x86 build configuration (line 286) uses gcc -m32 and g++ -m32 instead of cross-compilation toolchain commands like i686-linux-gnu-gcc. This is inconsistent with the ARM builds which install cross-compilation packages and use triplet-prefixed compilers.

If multilib compilation is the intent (using -m32 flags), consider using gcc-multilib g++-multilib as done in .github/workflows/linux.yml:400. If cross-compilation is the intent, update the build configuration to use i686-linux-gnu-gcc and i686-linux-gnu-g++.

Suggested change
sudo apt-get install -y crossbuild-essential-i386
sudo apt-get install -y gcc-multilib g++-multilib

Copilot uses AI. Check for mistakes.
run: |
sudo apt-get update
sudo apt-get install -y g++-multilib gcc-multilib
sudo apt-get install -y crossbuild-essential-i386
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions "Pin vcpkg to a specific revision in the CI workflow to reproduce and isolate the vcpkg test failure" but the actual change replaces x86 dependency packages. The description doesn't match the code change. Please update the PR description to accurately reflect that this changes x86 build dependencies.

Copilot uses AI. Check for mistakes.
Signed-off-by: Vitalii Koshura <[email protected]>
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings November 27, 2025 10:34
Copilot finished reviewing on behalf of AenBleidd November 27, 2025 10:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 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.

Signed-off-by: Vitalii Koshura <[email protected]>
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings November 27, 2025 10:50
Signed-off-by: Vitalii Koshura <[email protected]>
Signed-off-by: Vitalii Koshura <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Signed-off-by: Vitalii Koshura <[email protected]>
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings December 5, 2025 04:50
Copilot finished reviewing on behalf of AenBleidd December 5, 2025 04:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

sudo sed -i 's|priority:1|priority:100|' /etc/apt/apt-mirrors.txt
sudo apt-get update
sudo apt-get install -y g++-multilib gcc-multilib
sudo apt-get install -y g++-multilib gcc-multilib g++-i686-linux-gnu
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions installing gcc-i686-linux-gnu and binutils-i686-linux-gnu to "enforce 32-bit builds," but the actual code only adds g++-i686-linux-gnu. The gcc-i686-linux-gnu and binutils-i686-linux-gnu packages mentioned in the PR description are missing.

For consistency with the arm and arm64 dependency installations (lines 237 and 243), which install all three components (g++, gcc, and binutils), consider adding the missing packages:

sudo apt-get install -y g++-multilib gcc-multilib g++-i686-linux-gnu gcc-i686-linux-gnu binutils-i686-linux-gnu
Suggested change
sudo apt-get install -y g++-multilib gcc-multilib g++-i686-linux-gnu
sudo apt-get install -y g++-multilib gcc-multilib g++-i686-linux-gnu gcc-i686-linux-gnu binutils-i686-linux-gnu

Copilot uses AI. Check for mistakes.
Signed-off-by: Vitalii Koshura <[email protected]>
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings December 5, 2025 12:28
Copilot finished reviewing on behalf of AenBleidd December 5, 2025 12:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

Comment on lines 231 to 233
sudo apt-add-repository main
sudo apt-add-repository restricted
sudo apt-add-repository multiverse
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Adding main, restricted, and multiverse repositories individually is unusual. These Ubuntu repository components are typically already available in the default sources.list. Based on the pattern used in other workflow files (android.yml lines 52, 206), only the universe component typically needs to be added explicitly. Consider simplifying this to just sudo add-apt-repository universe unless there's a specific need for the other components.

Suggested change
sudo apt-add-repository main
sudo apt-add-repository restricted
sudo apt-add-repository multiverse

Copilot uses AI. Check for mistakes.
Comment on lines 230 to 233
sudo apt-add-repository universe
sudo apt-add-repository main
sudo apt-add-repository restricted
sudo apt-add-repository multiverse
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent command usage: apt-add-repository should be add-apt-repository to match the standard Ubuntu command and the pattern used elsewhere in this file (line 234) and other workflow files (e.g., android.yml lines 52, 206). The correct command is add-apt-repository.

Suggested change
sudo apt-add-repository universe
sudo apt-add-repository main
sudo apt-add-repository restricted
sudo apt-add-repository multiverse
sudo add-apt-repository universe
sudo add-apt-repository main
sudo add-apt-repository restricted
sudo add-apt-repository multiverse

Copilot uses AI. Check for mistakes.
Signed-off-by: Vitalii Koshura <[email protected]>
Copilot AI review requested due to automatic review settings December 5, 2025 22:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 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.

Signed-off-by: Vitalii Koshura <[email protected]>
@AenBleidd AenBleidd force-pushed the vko_debug_vcpkg_test_fail branch from 4ce2a0f to dfb9c7e Compare December 5, 2025 23:13
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.

1 participant