Skip to content

Add support for cpp in for compute#1773

Open
anthony-gomez-fastly wants to merge 12 commits intomainfrom
CDTOOL-1313-add-cpp-support-for-compute
Open

Add support for cpp in for compute#1773
anthony-gomez-fastly wants to merge 12 commits intomainfrom
CDTOOL-1313-add-cpp-support-for-compute

Conversation

@anthony-gomez-fastly
Copy link
Copy Markdown
Member

@anthony-gomez-fastly anthony-gomez-fastly commented May 1, 2026

Change summary

Adds ability to build C++ services and publish them

test service here

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  • Does your submission pass tests?

Changes to Core Features:

  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

@anthony-gomez-fastly anthony-gomez-fastly force-pushed the CDTOOL-1313-add-cpp-support-for-compute branch from 0e1f5e1 to fee97db Compare May 1, 2026 16:26
@anthony-gomez-fastly anthony-gomez-fastly marked this pull request as ready for review May 4, 2026 13:30
@anthony-gomez-fastly anthony-gomez-fastly requested a review from a team as a code owner May 4, 2026 13:30
Copy link
Copy Markdown
Member

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

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

One extraneous file, and a couple of 'nice to have' improvements. Otherwise this looks great.

Comment thread .github/workflows/pr_test.yml
Comment thread pkg/commands/compute/build_test.go Outdated
Comment thread fastly-dev Outdated
Copy link
Copy Markdown
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

Some of my findings are the same as Kevin's:

  1. Please remove the committed fastly-dev binary from the repo root.
  2. cpp is now exposed in compute init, but I do not see matching C++ starter-kit config. This could break the init flow if the starter-kit list is empty.
  3. The new config.CPP defaults do not appear to be added to the static/generated config, so real users may get empty C++ config values.
  4. WasmWasiTarget is defined but not used; either use it in the default build command or remove it for now.
  5. Add an init test for --language cpp, especially covering the no-starter-kit case.

@kpfleming
Copy link
Copy Markdown
Member

2. cpp is now exposed in compute init, but I do not see matching C++ starter-kit config. This could break the init flow if the starter-kit list is empty.

We don't have starter kits yet, that will happen next week. We do know what their URLs will be though, if those need to be included in the build.

@anthony-gomez-fastly
Copy link
Copy Markdown
Member Author

@philippschulte

  1. Please remove the committed fastly-dev binary from the repo root.
    Done in d5685ea
  2. cpp is now exposed in compute init, but I do not see matching C++ starter-kit config. This could break the init flow if the starter-kit list is empty.
    Kevin addressed this already, I have included a test that has the starter kit URL, which fails now but should pass once we're ready and the url is public.
  3. The new config.CPP defaults do not appear to be added to the static/generated config, so real users may get empty C++ config values.
    I added them to fastly.toml in c1dc0443
  4. WasmWasiTarget is defined but not used; either use it in the default build command or remove it for now.
    it is used in the build process from what i understand, as demonstarted in the config.
  5. Add an init test for --language cpp, especially covering the no-starter-kit case.
    done in c632e09

@anthony-gomez-fastly anthony-gomez-fastly dismissed stale reviews from philippschulte and kpfleming May 4, 2026 19:17

made changes

Comment thread pkg/commands/compute/init_test.go
Copy link
Copy Markdown
Member

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

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

One very small change and this is ready to go; the tests will fail until the starter kits are made public, so we can either just accept that for a couple of weeks or skip them for now.

Copy link
Copy Markdown
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. Point 4 still looks unresolved to me.

I see wasm_wasi_target is now present in .fastly/config.toml and mapped onto config.CPP.WasmWasiTarget, but I do not see that value being read anywhere in the C++ build path. The default build command still hardcodes wasm32-wasip1:

var CPPDefaultBuildCommand = fmt.Sprintf("clang++ -O3 --target=wasm32-wasip1 -o %s main.cpp", binWasmPath)

and Build() assigns that constant directly when [scripts.build] is missing.

So unless I am missing another code path, WasmWasiTarget is configured but currently unused.

@anthony-gomez-fastly
Copy link
Copy Markdown
Member Author

@philippschulte you're right, i missed the hardcoded value not being used, added support in 02965bcb

Copy link
Copy Markdown
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

Thank you!

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