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

src: add undici and acorn to process.versions #45621

Merged
merged 31 commits into from
Dec 18, 2022

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Nov 25, 2022

Attempted to update undici version when the update scripts are run
Refs: #45260
Refs: #45599

Have attempted to fix the issue in the approach as pointed out by @anonrig in #45599 would very much appreciate comments on if this is the correct direction, if yes will progress further on this

Thank you

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Nov 25, 2022
@debadree25 debadree25 marked this pull request as ready for review November 25, 2022 10:16
tools/update-undici.sh Outdated Show resolved Hide resolved
@RaisinTen
Copy link
Contributor

Fixes: #45260

Do you plan to include the version info for all the dependencies mentioned in the issue? If not, as suggested in #45599 (comment), it might be a good idea to change the Fixes to Refs.

@debadree25
Copy link
Member Author

Fixes: #45260

Do you plan to include the version info for all the dependencies mentioned in the issue? If not, as suggested in #45599 (comment), it might be a good idea to change the Fixes to Refs.

I do plan to include all the dependencies mentioned in the issue, if this approach to solving the problem is deemed appropiate

tools/update-undici.sh Outdated Show resolved Hide resolved
src/node_metadata.cc Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

@mhdawson Could you review -- I think this approach might not work/be accurate if undici is externalized via #44376.

@debadree25
Copy link
Member Author

One small help needed, is it possible to run a single test and check? i cant seem to find mention of that in the docs

@aduh95
Copy link
Contributor

aduh95 commented Nov 25, 2022

One small help needed, is it possible to run a single test and check? i cant seem to find mention of that in the docs

You can either run the file directly (node path/to/the/test.js), or use the Python test runner (python3 tools/test.py path/to/the/test).

@mhdawson
Copy link
Member

@richardlau that is a good point.

Looking at how we set the process.versions for lltthp in:

https://github.com/nodejs/node/blob/1a83ad6a693f851199608ae957ac5d4f76871485/src/node_metadata.cc#LL79C2-L79C2

I think we may have a similar problem with existing builtins as well.

I think to try to fix that for this one and others, it would likely be helpful to have it call a function like we do for OPENSSL:

#if HAVE_OPENSSL
  openssl = GetOpenSSLVersion();
#endif

and then work with the distros to have that function do the right thing.

@kapouer, @kasicka, @sgallagher have I missed something that makes this work ok in the distros? Or maybe there is already a solution in the distros that we can leverage?

@debadree25
Copy link
Member Author

debadree25 commented Nov 25, 2022

Have added proper tests and support for deps/acorn too as mentioned in the original issue, would appreciate any feedback to improve

Thank You!

@debadree25
Copy link
Member Author

debadree25 commented Nov 25, 2022

Also upon further investigation i see this approach cant be used for the remaining libraries, so updating the original PR message to Refs rather than fixes as @RaisinTen suggested for now

src/undici_version.h Outdated Show resolved Hide resolved
src/node_metadata.cc Outdated Show resolved Hide resolved
@debadree25 debadree25 changed the title src: added version information for undici in the process.versions src: added version information for undici and acorn in the process.versions Nov 26, 2022
@aduh95 aduh95 changed the title src: added version information for undici and acorn in the process.versions src: add undici and acorn to process.versions Nov 26, 2022
src/node_metadata.h Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Nov 26, 2022

Regarding the failed lint-commit-message: we can rebase your branch on top of main and reword the commit message of the first commit (I suggest src: add undici and acorn to process.versions), or can leave it as is and whoever lands this would fix it.

The other linter failures need to be addressed.

@debadree25
Copy link
Member Author

Fixing them

@debadree25
Copy link
Member Author

debadree25 commented Nov 26, 2022

@aduh95 have addressed the linter failures, I am leaving the commit message as is hoping it can be reworded at the time of landing I am little unsure about doing rebasing and amending

Also the previous change of allowing \ in the last line too had been rejected by the linter hence reverted back

@aduh95 aduh95 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 Nov 26, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@RaisinTen RaisinTen added request-ci Add this label to start a Jenkins CI on a PR. 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 Dec 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2022
@nodejs-github-bot

This comment was marked as outdated.

@aduh95 aduh95 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 18, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45621
✔  Done loading data for nodejs/node/pull/45621
----------------------------------- PR info ------------------------------------
Title      src:  add undici and acorn to `process.versions` (#45621)
Author     Debadree Chatterjee  (@debadree25)
Branch     debadree25:fix/process-versions-missing -> nodejs:main
Labels     c++, tools, author ready, needs-ci, commit-queue-squash
Commits    31
 - src: add undici and acorn to process.versions
 - tools: update script to use a more portable version of sed
 - tools: updated script to remove git commit
 - test: added test for undici version
 - src,tools: move the version definitions to a seprate file for undici
 - src,tools: added version information for acorn
 - test: added tests for acorn version
 - src: use proper version for undici
 - src: src/undici_version.h update file endline
 - src: src/node_metadata.cc update file endline
 - src: added header gaurds to version information header files
 - src: fixed linting issues in metadata files
 - tools: fixed linting issues in js test file
 - src: added files are automated comments
 - tools: add comments when rewriting the file
 - nit: fix comment
 - nit: fix comment
 - nit: update comments
 - src: bump up undici version
 - fix linting issues
 - lint: update test-process-versions.js
 - lint: test/parallel/test-process-versions.js
 - lint: src/node_metadata.h
 - test: make list alphabetical
 - test: add acorn to test version match
 - bump up undici version
 - src: remove undici version if externalised
 - fix lint
 - test: check if shared undici exists
 - lint fix again
 - test: small refactor
Committers 2
 - Debadree Chatterjee 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/45621
Refs: https://github.com/nodejs/node/issues/45260
Refs: https://github.com/nodejs/node/pull/45599
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Joyee Cheung 
Reviewed-By: Darshan Sen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45621
Refs: https://github.com/nodejs/node/issues/45260
Refs: https://github.com/nodejs/node/pull/45599
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Joyee Cheung 
Reviewed-By: Darshan Sen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Nov 2022 08:10:40 GMT
   ✔  Approvals: 4
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/45621#pullrequestreview-1194815159
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45621#pullrequestreview-1213353296
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/45621#pullrequestreview-1203625180
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/45621#pullrequestreview-1220900988
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-12-18T12:22:50Z: https://ci.nodejs.org/job/node-test-pull-request/48559/
- Querying data for job/node-test-pull-request/48559/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 45621
From https://github.com/nodejs/node
 * branch                  refs/pull/45621/merge -> FETCH_HEAD
✔  Fetched commits as ca2ec902e92f..c2e2d39db7c6
--------------------------------------------------------------------------------
[main 429ee171eb] src: add undici and acorn to process.versions
 Author: Debadree Chatterjee 
 Date: Fri Nov 25 13:37:25 2022 +0530
 3 files changed, 15 insertions(+), 3 deletions(-)
[main 9afee512dc] tools: update script to use a more portable version of sed
 Author: Debadree Chatterjee 
 Date: Fri Nov 25 17:19:09 2022 +0530
 1 file changed, 3 insertions(+), 1 deletion(-)
[main 23af79fe3f] tools: updated script to remove git commit
 Author: Debadree Chatterjee 
 Date: Fri Nov 25 19:14:52 2022 +0530
 1 file changed, 3 deletions(-)
[main 67bec99e8a] test: added test for undici version
 Author: Debadree Chatterjee 
 Date: Fri Nov 25 23:05:38 2022 +0530
 1 file changed, 18 insertions(+), 8 deletions(-)
[main a79011d3ee] src,tools: move the version definitions to a seprate file for undici
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 00:21:46 2022 +0530
 3 files changed, 4 insertions(+), 6 deletions(-)
 create mode 100644 src/undici_version.h
[main da39141304] src,tools: added version information for acorn
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 00:43:23 2022 +0530
 4 files changed, 9 insertions(+)
 create mode 100644 src/acorn_version.h
[main bc53088e31] test: added tests for acorn version
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 00:43:41 2022 +0530
 1 file changed, 5 insertions(+)
[main 63d61fc372] src: use proper version for undici
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 00:55:51 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main bab05cedf5] src: src/undici_version.h update file endline
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 10:10:10 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main df9f5258e3] src: src/node_metadata.cc update file endline
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 10:10:42 2022 +0530
 1 file changed, 1 deletion(-)
[main 3172f7a6e5] src: added header gaurds to version information header files
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 17:28:56 2022 +0530
 4 files changed, 14 insertions(+), 2 deletions(-)
[main 74132923dd] src: fixed linting issues in metadata files
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 17:29:38 2022 +0530
 1 file changed, 2 insertions(+), 2 deletions(-)
[main 2981d3f18b] tools: fixed linting issues in js test file
 Author: Debadree Chatterjee 
 Date: Sat Nov 26 17:30:18 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 7bc18f2541] src: added files are automated comments
 Author: Debadree Chatterjee 
 Date: Fri Dec 2 19:02:41 2022 +0530
 2 files changed, 4 insertions(+)
[main 4ab7133304] tools: add comments when rewriting the file
 Author: Debadree Chatterjee 
 Date: Fri Dec 2 19:03:34 2022 +0530
 2 files changed, 14 insertions(+), 8 deletions(-)
[main 26c33651bd] nit: fix comment
 Author: Debadree Chatterjee 
 Date: Sun Dec 4 15:18:01 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 36fe25f36d] nit: fix comment
 Author: Debadree Chatterjee 
 Date: Sun Dec 4 15:18:14 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 52333d3427] nit: update comments
 Author: Debadree Chatterjee 
 Date: Sun Dec 4 15:29:28 2022 +0530
 2 files changed, 2 insertions(+), 2 deletions(-)
[main ad88722782] src: bump up undici version
 Author: Debadree Chatterjee 
 Date: Sun Dec 4 15:30:03 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main a008bb4a28] fix linting issues
 Author: Debadree Chatterjee 
 Date: Mon Dec 12 12:59:13 2022 +0530
 1 file changed, 5 insertions(+), 6 deletions(-)
[main 860cd37557] lint: update test-process-versions.js
 Author: Debadree Chatterjee 
 Date: Mon Dec 12 13:13:04 2022 +0530
 1 file changed, 4 insertions(+), 3 deletions(-)
[main 1a78aaba08] lint: test/parallel/test-process-versions.js
 Author: Debadree Chatterjee 
 Date: Mon Dec 12 13:13:19 2022 +0530
 1 file changed, 2 insertions(+), 4 deletions(-)
[main 70f47e45a8] lint: src/node_metadata.h
 Author: Debadree Chatterjee 
 Date: Mon Dec 12 13:13:30 2022 +0530
 1 file changed, 2 insertions(+), 2 deletions(-)
[main 309629bbf8] test: make list alphabetical
 Author: Debadree Chatterjee 
 Date: Mon Dec 12 13:26:20 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 516b7fc854] test: add acorn to test version match
 Author: Debadree Chatterjee 
 Date: Mon Dec 12 13:28:07 2022 +0530
 1 file changed, 1 insertion(+)
[main a080957b48] bump up undici version
 Author: Debadree Chatterjee 
 Date: Tue Dec 13 10:43:20 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 12de57e50f] src: remove undici version if externalised
 Author: Debadree Chatterjee 
 Date: Thu Dec 15 21:59:44 2022 +0530
 2 files changed, 9 insertions(+), 1 deletion(-)
[main 6992b9219d] fix lint
 Author: Debadree Chatterjee 
 Date: Thu Dec 15 22:09:43 2022 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main cabd45bb5e] test: check if shared undici exists
 Author: Debadree Chatterjee 
 Date: Thu Dec 15 22:32:58 2022 +0530
 1 file changed, 16 insertions(+), 6 deletions(-)
[main 9042ced861] lint fix again
 Author: Debadree Chatterjee 
 Date: Thu Dec 15 22:34:45 2022 +0530
 1 file changed, 3 insertions(+), 3 deletions(-)
[main 5eb8832b45] test: small refactor
 Author: Debadree Chatterjee 
 Date: Thu Dec 15 22:35:55 2022 +0530
 1 file changed, 4 insertions(+), 4 deletions(-)
   ✔  Patches applied
There are 31 commits in the PR. Attempting to fixup everything into first commit.
[main 883bd6aea2] src: add undici and acorn to process.versions
 Author: Debadree Chatterjee 
 Date: Fri Nov 25 13:37:25 2022 +0530
 7 files changed, 72 insertions(+), 1 deletion(-)
 create mode 100644 src/acorn_version.h
 create mode 100644 src/undici_version.h
   ✖  Git found no trailers in the original commit message, but 'Refs: https://github.com/nodejs/node/pull/45599' is present and should be a trailer.
https://github.com/nodejs/node/actions/runs/3725468283

@aduh95 aduh95 merged commit 9d1d948 into nodejs:main Dec 18, 2022
@aduh95
Copy link
Contributor

aduh95 commented Dec 18, 2022

Landed in 9d1d948

@debadree25
Copy link
Member Author

Thank you so much to everyone for their time ❤️

targos pushed a commit that referenced this pull request Jan 1, 2023
Fixes: #45260
Refs: #45599
Refs: #45260
PR-URL: #45621
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
Fixes: #45260
Refs: #45599
Refs: #45260
PR-URL: #45621
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Fixes: #45260
Refs: #45599
Refs: #45260
PR-URL: #45621
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this 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. 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.

8 participants