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

deps, src: simplifying base64 encoding #52714

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

lemire
Copy link
Member

@lemire lemire commented Apr 26, 2024

Following recent work by @anonrig, we no longer have use for existing base64 encoding code. This PR, if merged, will drop much of the base64 encoding code.

It should be otherwise inconsequential.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg

@lemire lemire requested a review from anonrig April 26, 2024 19:15
@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Apr 26, 2024
@lemire lemire force-pushed the simplifying_base64_encoding branch 2 times, most recently from 280494e to 8297c11 Compare April 26, 2024 19:40
@richardlau richardlau added the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 26, 2024
@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2024
@meyfa
Copy link
Contributor

meyfa commented Apr 27, 2024

Awesome!

Nit: I think the commit message needs to be amended to start with an imperative verb, according to the guidelines.

@lemire lemire force-pushed the simplifying_base64_encoding branch from 842e9e1 to 7a07c7e Compare April 27, 2024 15:31
@lemire
Copy link
Member Author

lemire commented Apr 27, 2024

Nit: I think the commit message needs to be amended to start with an imperative verb, according to the guidelines.

Fixed.

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2024
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

image
nice

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Please run tools/license-builder.sh to update the license

@lemire
Copy link
Member Author

lemire commented Apr 29, 2024

@targos Good catch! Ran it.

@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/52714
✔  Done loading data for nodejs/node/pull/52714
----------------------------------- PR info ------------------------------------
Title      deps, src: simplifying base64 encoding (#52714)
Author     Daniel Lemire  (@lemire)
Branch     lemire:simplifying_base64_encoding -> nodejs:main
Labels     tools, baking-for-lts, needs-ci, dependencies
Commits    2
 - deps,src: simplify base64 encoding
 - fix: tools/license-builder.sh
Committers 1
 - Daniel Lemire 
PR-URL: https://github.com/nodejs/node/pull/52714
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Moshe Atlow 
Reviewed-By: Michaël Zasso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Marco Ippolito 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52714
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Moshe Atlow 
Reviewed-By: Michaël Zasso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Marco Ippolito 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 26 Apr 2024 19:15:25 GMT
   ✔  Approvals: 5
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/52714#pullrequestreview-2025987672
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/52714#pullrequestreview-2026906558
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/52714#pullrequestreview-2028772611
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/52714#pullrequestreview-2028491743
   ✔  - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/52714#pullrequestreview-2028604304
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-04-28T06:18:56Z: https://ci.nodejs.org/job/node-test-pull-request/58770/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - fix: tools/license-builder.sh
- Querying data for job/node-test-pull-request/58770/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8881428267

@richardlau richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 30, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 30, 2024
@nodejs-github-bot nodejs-github-bot merged commit 6aa9047 into nodejs:main Apr 30, 2024
72 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 6aa9047

aduh95 pushed a commit that referenced this pull request Apr 30, 2024
PR-URL: #52714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this pull request May 8, 2024
PR-URL: nodejs#52714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau added a commit to richardlau/node-1 that referenced this pull request Jun 13, 2024
The `base64` dependency was previously removed along with the update
script (`tools/dep_updaters/update-base64.sh`) but the generated header,
`src/base64_version.h` was left behind and `process.versions` was still
listing the last version of `base64` that was included in Node.js before
it was removed

Refs: nodejs#52714
@richardlau
Copy link
Member

When this lands in LTS, it should land with the fixup for process.versions: #53442.

nodejs-github-bot pushed a commit that referenced this pull request Jun 15, 2024
The `base64` dependency was previously removed along with the update
script (`tools/dep_updaters/update-base64.sh`) but the generated header,
`src/base64_version.h` was left behind and `process.versions` was still
listing the last version of `base64` that was included in Node.js before
it was removed

Refs: #52714
PR-URL: #53442
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
targos pushed a commit that referenced this pull request Jun 20, 2024
The `base64` dependency was previously removed along with the update
script (`tools/dep_updaters/update-base64.sh`) but the generated header,
`src/base64_version.h` was left behind and `process.versions` was still
listing the last version of `base64` that was included in Node.js before
it was removed

Refs: #52714
PR-URL: #53442
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
PR-URL: nodejs#52714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
The `base64` dependency was previously removed along with the update
script (`tools/dep_updaters/update-base64.sh`) but the generated header,
`src/base64_version.h` was left behind and `process.versions` was still
listing the last version of `base64` that was included in Node.js before
it was removed

Refs: nodejs#52714
PR-URL: nodejs#53442
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
PR-URL: nodejs#52714
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
The `base64` dependency was previously removed along with the update
script (`tools/dep_updaters/update-base64.sh`) but the generated header,
`src/base64_version.h` was left behind and `process.versions` was still
listing the last version of `base64` that was included in Node.js before
it was removed

Refs: nodejs#52714
PR-URL: nodejs#53442
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants