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

Assorted CI dependency cleanup #1359

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Assorted CI dependency cleanup #1359

merged 2 commits into from
Oct 31, 2023

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Oct 27, 2023

See details in the commit descriptions. Note: This removes esbuild which appears to be unused but might be useful for future JS/TS work, feel free to add it back if needed or lmk if you want to keep it.

@fhanau fhanau requested review from a team as code owners October 27, 2023 17:38
@fhanau fhanau requested a review from dom96 October 27, 2023 17:38
@jasnell
Copy link
Member

jasnell commented Oct 27, 2023

@mrbbot .. are y'all using the esbuild dependency here?

@mrbbot
Copy link
Contributor

mrbbot commented Oct 30, 2023

We use esbuild in the npm release workflow...

- name: Build node-install
run: npx esbuild npm/lib/node-install.ts --outfile=npm/workerd/install.js --bundle --target=node16 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning
env:
WORKERD_VERSION: ${{ needs.version.outputs.version }}
LATEST_COMPATIBILITY_DATE: ${{ needs.version.outputs.date }}
- name: Build node-shim
run: npx esbuild npm/lib/node-shim.ts --outfile=npm/workerd/bin/workerd --bundle --target=node16 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning
env:
WORKERD_VERSION: ${{ needs.version.outputs.version }}
LATEST_COMPATIBILITY_DATE: ${{ needs.version.outputs.date }}
- name: Build node-path
run: npx esbuild npm/lib/node-path.ts --outfile=npm/workerd/lib/main.js --bundle --target=node16 --define:LATEST_COMPATIBILITY_DATE="\"${LATEST_COMPATIBILITY_DATE}\"" --define:WORKERD_VERSION="\"${WORKERD_VERSION}\"" --platform=node --external:workerd --log-level=warning
env:
WORKERD_VERSION: ${{ needs.version.outputs.version }}
LATEST_COMPATIBILITY_DATE: ${{ needs.version.outputs.date }}

...but these are all prefixed with npx, and I can't see a pnpm install in that workflow, so I'm not actually sure if the version in package.json is used. Ideally it would be though.

These are not needed for the build itself, but not having the right deps broke bazel query.
This fixes warnings about deprecated node versions/"set-output" commands on
CI runs and allows us to remove one instance of GITHUB_TOKEN.
@fhanau
Copy link
Collaborator Author

fhanau commented Oct 30, 2023

Sounds like there is at least potential value in retaining esbuild... Updated the PR to drop that patch for now.

@fhanau fhanau merged commit a27dbee into main Oct 31, 2023
11 checks passed
@fhanau fhanau deleted the felix/ci-dep-cleanup branch October 31, 2023 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants