Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed container build trigger and cleanup containers #875

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

AhmedBM
Copy link
Contributor

@AhmedBM AhmedBM commented Feb 14, 2025

Description

  • Fixed container build workflow trigger to dev
  • Removed arm/arm64 container definitions
  • Fixed debian-9 to use debian snapshot mirrors

Checklist

  • I have read the contribution guidelines.
  • I added unit-tests to validate my changes. All unit tests are passing.
  • I have merged the latest dev branch prior to this PR submission.
  • I ran pre-commit on my changes prior to this PR submission.
  • I submitted this PR against the dev branch.

@AhmedBM AhmedBM requested a review from a team as a code owner February 14, 2025 19:38
Copy link

github-actions bot commented Feb 14, 2025

Test Results

 44 files  ±0   44 suites  ±0   37m 6s ⏱️ - 2m 56s
  4 tests ±0    4 ✅ ±0   0 💤 ±0  0 ❌ ±0 
176 runs  ±0  154 ✅ ±0  22 💤 ±0  0 ❌ ±0 

Results for commit 595cd56. ± Comparison against base commit 81bd796.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@MariusNi MariusNi left a comment

Choose a reason for hiding this comment

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

:shipit:

@robertwoj-microsoft
Copy link
Contributor

We are failing some tests, for example here:

------
 > [9/9] RUN cd rapidjson && cmake . && cmake --build . --parallel --target install && rm -rf /git/rapidjson:
46.62 /git/rapidjson/include/rapidjson/document.h:2454:24: error: 'void* memcpy(void*, const void*, size_t)' forming offset [5, 12] is out of the bounds [0, 4] [-Werror=array-bounds]
46.62              std::memcpy(str, s, s.length * sizeof(Ch));
46.62              ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Seems we use rapidjson directly from master and have unpredictable builds. Have you considered freezing the library version similarly to how we do for cmake or gtest? I know the library does not really provide tags and is not versioned properly, but we could tie to a specific git ref. What do you think?

@AhmedBM
Copy link
Contributor Author

AhmedBM commented Feb 20, 2025

We are failing some tests, for example here:

------
 > [9/9] RUN cd rapidjson && cmake . && cmake --build . --parallel --target install && rm -rf /git/rapidjson:
46.62 /git/rapidjson/include/rapidjson/document.h:2454:24: error: 'void* memcpy(void*, const void*, size_t)' forming offset [5, 12] is out of the bounds [0, 4] [-Werror=array-bounds]
46.62              std::memcpy(str, s, s.length * sizeof(Ch));
46.62              ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Seems we use rapidjson directly from master and have unpredictable builds. Have you considered freezing the library version similarly to how we do for cmake or gtest? I know the library does not really provide tags and is not versioned properly, but we could tie to a specific git ref. What do you think?

Yes, we do for some of the older distros but we really should be normalized across all distros. Ill see if i can lock down a version that works across all our supported distros

The issue is with distros where GCC>=9 (see Tencent/rapidjson#1700)

@MariusNi MariusNi self-requested a review February 21, 2025 00:26
Copy link
Contributor

@MariusNi MariusNi left a comment

Choose a reason for hiding this comment

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

LGTM

@AhmedBM AhmedBM merged commit 7d3c007 into dev Feb 21, 2025
129 checks passed
@AhmedBM AhmedBM deleted the ahbenmes/container_cleanup branch February 21, 2025 17:00
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.

4 participants