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.

Sign up for GitHub

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

Jump to bottom

process: print versions by sort #46428

Merged
merged 5 commits into from
Feb 18, 2023
Merged

process: print versions by sort #46428

merged 5 commits into from
Feb 18, 2023

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Jan 30, 2023 •
edited
Loading

Fixes: #45630

@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. labels Jan 30, 2023
@himself65
Copy link
Member Author

I'm trying to use std::pair<name, version_number> then sort and create a new v8::Object

@himself65 himself65 changed the title src: sort version keys process: print versions by sort Jan 30, 2023
@himself65 himself65 marked this pull request as ready for review January 30, 2023 20:55
@himself65 himself65 added the process Issues and PRs related to the process subsystem. label Jan 30, 2023
@himself65 himself65 mentioned this pull request Jan 30, 2023
Closed
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@himself65 himself65 requested a review from addaleax January 30, 2023 21:19
@himself65 himself65 force-pushed the 20230130-sort-versions branch from fd4a77b to affc744 Compare January 30, 2023 21:28
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
addaleax
addaleax approved these changes Jan 30, 2023
View reviewed changes
src/node_process_object.cc Outdated Show resolved Hide resolved
src/node_process_object.cc Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Jan 31, 2023
View reviewed changes
src/node_process_object.cc Outdated Show resolved Hide resolved
@himself65
Copy link
Member Author

I'm confused that why build CI all crashed today

@jasnell
Copy link
Member

jasnell commented Jan 31, 2023

There was a GitHub issue with tarball sha's today that caused stuff all over the place to break. The rolled it back this afternoon. Pretty sure that's what hit us today.

@RaisinTen
Copy link
Contributor

Could you also add a test?

@himself65
Copy link
Member Author

Could you also add a test?

I will do it later

@himself65
Copy link
Member Author

ive updated the test case

@himself65 himself65 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2023
@himself65 himself65 requested a review from RaisinTen January 31, 2023 16:10
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
bnoordhuis
bnoordhuis reviewed Feb 3, 2023
View reviewed changes
src/node_process_object.cc Outdated Show resolved Hide resolved
src/node_process_object.cc Outdated Show resolved Hide resolved
src/node_report.cc Outdated
@@ -793,11 +793,24 @@ static void PrintComponentVersions(JSONWriter* writer) {
std::stringstream buf;

writer->json_objectstart("componentVersions");
std::vector<std::pair<std::string, std::string>> versions_vec;
Copy link
Member

Choose a reason for hiding this comment

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

Performance isn't as important here but consider using the same technique for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think json_keyvalue(k, v) has no implement for std::string_view

@himself65 himself65 force-pushed the 20230130-sort-versions branch from cdc159d to a6df4cb Compare February 3, 2023 22:59
@himself65 himself65 force-pushed the 20230130-sort-versions branch from 0a314ab to 73a1163 Compare February 11, 2023 16:34
@himself65
Copy link
Member Author

nodejs ci is disabled for now. how can I run it before merge the pr

@richardlau
Copy link
Member

nodejs ci is disabled for now. how can I run it before merge the pr

You'll have to wait until the embargo is lifted and CI reenabled.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49616/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49699/

@addaleax addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023
@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 Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/46428
✔  Done loading data for nodejs/node/pull/46428
----------------------------------- PR info ------------------------------------
Title      process: print versions by sort (#46428)
Author     Himself65  (@Himself65)
Branch     Himself65:20230130-sort-versions -> nodejs:main
Labels     c++, process, author ready, needs-ci
Commits    5
 - process: print versions by sort
 - fixup!: code
 - fixup!: code
 - fixup!: format
 - fixup!: format
Committers 1
 - himself65 
PR-URL: https://github.com/nodejs/node/pull/46428
Fixes: https://github.com/nodejs/node/issues/45630
Reviewed-By: Anna Henningsen 
Reviewed-By: James M Snell 
Reviewed-By: Darshan Sen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46428
Fixes: https://github.com/nodejs/node/issues/45630
Reviewed-By: Anna Henningsen 
Reviewed-By: James M Snell 
Reviewed-By: Darshan Sen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 30 Jan 2023 18:42:25 GMT
   ✔  Approvals: 3
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/46428#pullrequestreview-1304619046
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/46428#pullrequestreview-1282238933
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/46428#pullrequestreview-1284199496
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-02-18T17:34:16Z: https://ci.nodejs.org/job/node-test-pull-request/49699/
- Querying data for job/node-test-pull-request/49699/
   ✔  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 46428
From https://github.com/nodejs/node
 * branch                  refs/pull/46428/merge -> FETCH_HEAD
✔  Fetched commits as 4ba860ee693d..73a1163802da
--------------------------------------------------------------------------------
[main 2d9275ad6b] process: print versions by sort
 Author: himself65 
 Date: Fri Feb 3 17:04:23 2023 -0600
 3 files changed, 42 insertions(+), 9 deletions(-)
[main ab24f3f61c] fixup!: code
 Author: himself65 
 Date: Fri Feb 3 17:19:54 2023 -0600
 2 files changed, 21 insertions(+), 13 deletions(-)
[main 66f67104ab] fixup!: code
 Author: himself65 
 Date: Sat Feb 4 15:10:17 2023 -0600
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 881db7bec2] fixup!: format
 Author: himself65 
 Date: Sat Feb 4 15:10:32 2023 -0600
 1 file changed, 2 insertions(+), 1 deletion(-)
[main cb6ee4df18] fixup!: format
 Author: himself65 
 Date: Sat Feb 4 15:40:33 2023 -0600
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 5 commits in the PR. Attempting autorebase.
Rebasing (2/10)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

process: print versions by sort


Co-authored-by: James M Snell jasnell@gmail.com

Co-authored-by: Anna Henningsen github@addaleax.net

PR-URL: #46428

Fixes: #45630

Reviewed-By: Anna Henningsen anna@addaleax.net

Reviewed-By: James M Snell jasnell@gmail.com

Reviewed-By: Darshan Sen raisinten@gmail.com


[detached HEAD 6420f6391c] process: print versions by sort

Author: himself65 himself65@toeverything.info

Date: Fri Feb 3 17:04:23 2023 -0600

3 files changed, 42 insertions(+), 9 deletions(-)

Rebasing (3/10)

Rebasing (4/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup!: code


PR-URL: #46428

Fixes: #45630

Reviewed-By: Anna Henningsen anna@addaleax.net

Reviewed-By: James M Snell jasnell@gmail.com

Reviewed-By: Darshan Sen raisinten@gmail.com


[detached HEAD c4c7d3f07d] fixup!: code

Author: himself65 himself65@toeverything.info

Date: Fri Feb 3 17:19:54 2023 -0600

2 files changed, 21 insertions(+), 13 deletions(-)

Rebasing (5/10)

Rebasing (6/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup!: code


PR-URL: #46428

Fixes: #45630

Reviewed-By: Anna Henningsen anna@addaleax.net

Reviewed-By: James M Snell jasnell@gmail.com

Reviewed-By: Darshan Sen raisinten@gmail.com


[detached HEAD 5b2476cd8a] fixup!: code

Author: himself65 himself65@toeverything.info

Date: Sat Feb 4 15:10:17 2023 -0600

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (7/10)

Rebasing (8/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup!: format


PR-URL: #46428

Fixes: #45630

Reviewed-By: Anna Henningsen anna@addaleax.net

Reviewed-By: James M Snell jasnell@gmail.com

Reviewed-By: Darshan Sen raisinten@gmail.com


[detached HEAD e6608b05c5] fixup!: format

Author: himself65 himself65@toeverything.info

Date: Sat Feb 4 15:10:32 2023 -0600

1 file changed, 2 insertions(+), 1 deletion(-)

Rebasing (9/10)

Rebasing (10/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup!: format


PR-URL: #46428

Fixes: #45630

Reviewed-By: Anna Henningsen anna@addaleax.net

Reviewed-By: James M Snell jasnell@gmail.com

Reviewed-By: Darshan Sen raisinten@gmail.com


[detached HEAD 500f156097] fixup!: format

Author: himself65 himself65@toeverything.info

Date: Sat Feb 4 15:40:33 2023 -0600

1 file changed, 1 insertion(+), 1 deletion(-)


Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/4212781930

@addaleax addaleax 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. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 070246f into nodejs:main Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 070246f

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@himself65 @jasnell
@addaleax @MylesBorins
process: print versions by sort
a22f1f9 
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Anna Henningsen <github@addaleax.net>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
@himself65 himself65 deleted the 20230130-sort-versions branch February 19, 2023 04:58
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
@himself65 @jasnell
@addaleax @MylesBorins
process: print versions by sort
152a3c7 
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Anna Henningsen <github@addaleax.net>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@anonrig anonrig mentioned this pull request Apr 6, 2023
Closed
anonrig pushed a commit to anonrig/node that referenced this pull request Apr 6, 2023
@himself65 @jasnell
@addaleax @anonrig
process: print versions by sort
574f464 
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Anna Henningsen <github@addaleax.net>
PR-URL: nodejs#46428
Fixes: nodejs#45630
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@anonrig anonrig added the backport-open-v18.x Indicate that the PR has an open backport. label Apr 6, 2023
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@himself65 @jasnell
@addaleax @danielleadams
process: print versions by sort
c231aae 
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Anna Henningsen <github@addaleax.net>
PR-URL: #46428
Fixes: #45630
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
@richardlau richardlau added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@RaisinTen RaisinTen RaisinTen approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. 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. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The version list should be sorted by name
8 participants
@himself65 @nodejs-github-bot @jasnell @RaisinTen @richardlau @bnoordhuis @addaleax @anonrig