Skip to content

[envpool] finish phase-1 native dependency upgrades - #335

Merged
Trinkle23897 merged 5 commits into
mainfrom
jiayi/envpool-phase1-native-upgrades
Mar 21, 2026
Merged

[envpool] finish phase-1 native dependency upgrades#335
Trinkle23897 merged 5 commits into
mainfrom
jiayi/envpool-phase1-native-upgrades

Conversation

@Trinkle23897

Copy link
Copy Markdown
Collaborator

Summary

  • Problem: phase-1 native dependencies were still pinned to older releases, and the repo's current lint/toolchain setup did not cleanly survive the native upgrade work on Linux.
  • Scope: upgrade the remaining phase-1 native third-party pieces used by the current repo and apply only the mechanical code/build cleanups needed to keep Bazel, lint, and tests green on dev.
  • Outcome: the native dependency pass now covers NASM, libjpeg-turbo, Boost, and the vendored ThreadPool compatibility edge, with make lint and full Bazel tests passing on dev-0.

This finishes the first native-upgrade pass without pulling in the larger benchmark modernization or Bazel rules migration work.

Technical Details

  • Approach: vendor NASM 3.01's Linux config header, migrate the libjpeg-turbo BUILD to the 3.x source layout, patch vendored ThreadPool for std::invoke_result_t, pin repo lint to clang-tidy-18, and make small semantics-preserving code cleanups where the newer lint stack flagged issues.
  • Code pointers:
    • envpool/workspace0.bzl: native archive versions, NASM/threadpool wiring, and updated fetch metadata.
    • third_party/jpeg/jpeg.BUILD: libjpeg-turbo 3.x layout, generated headers, and SIMD rule updates.
    • third_party/nasm/config.h: checked-in Linux configure output used to keep NASM 3.01 reproducible under Bazel.
    • third_party/threadpool/invoke_result.patch: C++17 compatibility patch replacing std::result_of.
    • Makefile: clang-tidy-18 install/pin wrapper used by repo lint.
  • Notes: the first full make bazel-test on dev-0 hit one //envpool/dummy:dummy_envpool_test flake; the single-target rerun, --runs_per_test=3, and a second full make bazel-test all passed.

Test Plan

Automated

  • brix ssh dev-0 -C -- 'exec bash -il -c "cd /root/code/envpool && make lint"': passed
  • brix ssh dev-0 -C -- 'exec bash -il -c "cd /root/code/envpool && make bazel-test"': passed on rerun (30/30)
  • brix ssh dev-0 -C -- 'exec bash -il -c "cd /root/code/envpool && USE_BAZEL_VERSION=6.0.0 /root/code/openai/project/dotslash-gen/bin/bazelisk test //envpool/dummy:dummy_envpool_test --config=test --test_output=all --nocache_test_results"': passed
  • brix ssh dev-0 -C -- 'exec bash -il -c "cd /root/code/envpool && USE_BAZEL_VERSION=6.0.0 /root/code/openai/project/dotslash-gen/bin/bazelisk test //envpool/dummy:dummy_envpool_test --config=test --test_output=errors --nocache_test_results --runs_per_test=3"': passed

Suggested Manual

  • git diff origin/main...HEAD: review the third-party archive/version changes and BUILD-file migrations.
  • Review third_party/jpeg/jpeg.BUILD and third_party/nasm/config.h: confirm the checked-in NASM config header and libjpeg 3.x Bazelization strategy are acceptable.

@Trinkle23897
Trinkle23897 merged commit 0099d73 into main Mar 21, 2026
2 of 4 checks passed
@Trinkle23897
Trinkle23897 deleted the jiayi/envpool-phase1-native-upgrades branch March 21, 2026 17:17

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6cc1013c5d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Makefile

clang-tidy-install:
command -v clang-tidy || sudo apt-get install -y clang-tidy
command -v $(CLANG_TIDY_BIN) || sudo apt-get install -y $(CLANG_TIDY_BIN)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid hard-requiring clang-tidy-18 in Ubuntu 22.04 setups

Checked docker/dev.dockerfile, docker/dev-cn.dockerfile, and the documented make lint flow: the repo’s stock dev images are still based on Ubuntu 22.04 and only install the unversioned clang-tidy package, but this change makes make clang-tidy require clang-tidy-18 specifically. On those images, command -v clang-tidy-18 will fail and the fallback apt-get install clang-tidy-18 is not available from the default 22.04 repos, so the documented lint workflow stops working even before Bazel runs.

Useful? React with 👍 / 👎.

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