Skip to content

feat: remove vendored aws-lambda-cpp dependency - #633

Open
maxday wants to merge 12 commits into
mainfrom
maxday/remove-deps
Open

feat: remove vendored aws-lambda-cpp dependency#633
maxday wants to merge 12 commits into
mainfrom
maxday/remove-deps

Conversation

@maxday

@maxday maxday commented Aug 18, 2026

Copy link
Copy Markdown
Member
  1. Removes the vendored copy of aws-lambda-cpp (the aws-lambda-cpp-0.2.7/ source tree, including a bundled googletest) that lived under src/main/jni/deps/ and consumes the dependency as the prebuilt static library published on the upstream GitHub release instead of compiling it from source during the RIC native build. This deletes ~34k lines of vendored third-party code and drops CMake from the build entirely.

  2. The OS integration test matrix is modernized: end-of-life distributions are removed and the rest bumped to supported releases. CentOS is dropped entirely, Debian moves from buster/bullseye to bookworm/trixie (switching to the modern signed-by keyring approach), and Ubuntu moves from 18.04/20.04/21.10 to 22.04/24.04.

    [refs]

  3. The OS integration tests now run in PR, parallelized as a {buildspec} x {arch} matrix with each arch on its own native runner instead of QEMU emulation. test_all.sh accepts a single buildspec so CI can shard per OS and arch.

  4. A reusable docker-retry.sh helper wraps every image pull with exponential backoff and full jitter to avoid public.ecr.aws rate limits. It also fixes the CI agent image never being loaded into the docker image store (--load).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.79%. Comparing base (a32b1e4) to head (d19c1b9).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #633      +/-   ##
============================================
+ Coverage     65.38%   65.79%   +0.40%     
- Complexity      212      213       +1     
============================================
  Files            34       34              
  Lines           991      991              
  Branches        143      143              
============================================
+ Hits            648      652       +4     
+ Misses          290      287       -3     
+ Partials         53       52       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maxday
maxday marked this pull request as ready for review August 19, 2026 03:17
@darklight3it
darklight3it self-requested a review August 19, 2026 16:20

@darklight3it darklight3it left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you run make test-integ before merging. This is a big change and I'd like to have it tested with something more than unit test before we reach mainline.

@maxday

maxday commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

@darklight3it actually this check should run at the PR level, we should not rely on running this manually on best effort for every PR so I've included it in this PR, cleaning also EOL distributions (more details on the description)

@maxday maxday mentioned this pull request Aug 20, 2026
@darklight3it

darklight3it commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

@maxday I agree with that. But I have a couple of suggestions:

  1. Integration tests should be runnable only by maintainers.
  2. @fabisev created a new workflow run-integration-test that I think should be transformed into an action so we can call the integration test action in many different places.
  3. I feel we are going in a direction in which testing is becoming more and more complex. I would likely defer major works to a document reshaping how tests are done here before committing to a solution.

#!/bin/bash
# Copyright 2026 Amazon.com, Inc. or its affiliates. All Rights Reserved.

set -uo pipefail

@darklight3it darklight3it Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this? Can you elaborate on this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, now the matrix integration test is triggering 11 jobs at the same time (combo arch / distribution). One of the first steps of this is to pull an image from a public registry without authentication. The result is that we're getting throttled. This script allows us to retry when we receive a throttling status code. It was already the case before this PR, and we used to just retry. This is a more robust solution IMO.

The other solution is to add authentication, which raises the pull concurrency limit and will make the throttling go away, but this adds complexity for external contributors who fork this repo, as it won't come with the secrets and authentication details.

@maxday

maxday commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

@darklight3it

Integration tests should be runnable only by maintainers.

Why? Those integration tests don't need any credentials, they're just building the RIC across different distributions/architectures + perform a hello world invoke in a containerized environment via RIE. It's great value at PR time to detect any distribution-related regression IMO.

@fabisev created a new workflow run-integration-test that I think should be transformed into an action so we can
call the integration test action in many different places.

Those a tests that require credentials, it's a different use case (sam deploy)

I feel we are going in a direction in which testing is becoming more and more complex. I would likely defer major works to a document reshaping how tests are done here before committing to a solution.

I don't feel this is major work for testing, here we're just calling an existing (and totally forgotten) make target at PR time to increase test coverage and detect arch/distribution specific bugs.

- (cd aws-lambda-java-core && mvn install)
- (cd aws-lambda-java-serialization && mvn install)
- (cd aws-lambda-java-runtime-interface-client && mvn install)
- (cd aws-lambda-java-runtime-interface-client && mvn install -DargLineForReflectionTestOnly="")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why does -DargLineForReflectionTestOnly="" only show up in the debian and ubuntu buildspecs? mvn runs in the corretto8 agent either way, so I'd expect all of them to need it or none. Worth a comment either way.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it's the other way around, it's already set on main for all other distributions (ie: https://github.com/aws/aws-lambda-java-libs/blob/main/aws-lambda-java-runtime-interface-client/test/integration/codebuild/buildspec.os.amazonlinux.2.yml#L46)
we have forgotten to add it here

env:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

integration-test:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we add a join job for this matrix like smoke-test has (needs + if: always())? Branch protection against individual matrix job names gets painful. Also this runs all 11 combos on every PR with no needs or path filter — intentional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done, will add the branch protection once it reaches main not to block other PRs

make install
COPY ./deps/aws-lambda-cpp/include /src/deps/artifacts/include
COPY ./deps/aws-lambda-cpp/lib/libaws-lambda-runtime.a /src/deps/artifacts/lib/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Curious how the alpine leg works with a glibc-built .a, since alpine doesn't have glibc (musl) — before this we compiled from source inside the container, so is musl's glibc symbol compat enough here or should upstream publish a musl variant too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The prebuilt .a is just an archive of relocatable object files, so what matters isn't the libc it was built on but which external symbols its objects actually reference and none of them are glibc-specific.

The final .so in the musl leg still links against the musl-built static curl and musl libc inside the Alpine container, with libstdc++/libgcc statically linked, so nothing glibc leaks in on that side either.

Code used

curl -fsSL -o lib.a https://github.com/awslabs/aws-lambda-cpp/releases/download/v1.0.1/libaws-lambda-runtime-x86_64.a
ar x lib.a

for f in *.o; do readelf -sW "$f"; done | grep -c "@GLIBC_"
# returns nothins

This is also why the tests are passing

@darklight3it

Copy link
Copy Markdown
Collaborator

@maxday

Those a tests that require credentials, it's a different use case (sam deploy)

mmm I was thinking they were using both SAM. If no it's ok.

I don't feel this is major work for testing, here we're just calling an existing (and totally forgotten) make target at PR time to increase test coverage and detect arch/distribution specific bugs.

Still too much shellscript here. It's genuinely distressing. I'm approving here for the greater benefit of the client but we should do something about it.

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.

3 participants