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

src: fix node_version.h #50375

Merged
merged 1 commit into from
Oct 24, 2023
Merged

src: fix node_version.h #50375

merged 1 commit into from
Oct 24, 2023

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 24, 2023 •
edited
Loading

  • Fix the version for main branch as 22.0.0
  • Fix the NODE_VERSION_IS_RELEASE flag for the main branch

This would allow test/parallel/test-release-changelog.js
to pass again on the main branch.

Fixes: #50373

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 24, 2023
@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 24, 2023
@github-actions
Copy link
Contributor

Fast-track has been requested by @joyeecheung. Please 👍 to approve.

targos
targos requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@targos targos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't the version actually be 22.0.0?

@richardlau
Copy link
Member

Shouldn't the version actually be 22.0.0?

Yes, I think we're missing a step for semver major releases in the release process to bump the major version on main.

@targos
Copy link
Member

targos commented Oct 24, 2023

Yeah, there are two bugs introduced in the file during the release of v21.0.0:

  • major should be set to 22
  • is_release should be set to 0

image

@joyeecheung
Copy link
Member Author

Something is wrong with the test, if I change it to 22.0.0:

❯ out/Release/node test/parallel/test-release-changelog.js
node:fs:453
    return binding.readFileUtf8(path, stringToFlags(options.flag));
                   ^

Error: ENOENT: no such file or directory, open '/Users/joyee/projects/node/doc/changelogs/CHANGELOG_V22.md'
    at Object.readFileSync (node:fs:453:20)
    at Object.<anonymous> (/Users/joyee/projects/node/test/parallel/test-release-changelog.js:42:24)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12)
    at node:internal/main/run_main_module:28:49 {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/joyee/projects/node/doc/changelogs/CHANGELOG_V22.md'
}

@targos
Copy link
Member

targos commented Oct 24, 2023

@joyeecheung you probably need to fix the IS_RELEASE macro at the same time. I think the bug slipped through because both mistakes happened.

@joyeecheung
Copy link
Member Author

Maybe we should just skip the test when it's not run on a release?

@joyeecheung
Copy link
Member Author

Oh, I see, it was supposed to skip, but NODE_VERSION_IS_RELEASE wasn't updated either.

@joyeecheung
src: fix node_version.h
c770105 
- Fix the version for main branch as 22.0.0
- Fix the NODE_VERSION_IS_RELEASE flag for the main branch

This would allow test/parallel/test-release-changelog.js
to pass again on the main branch.
@richardlau richardlau mentioned this pull request Oct 24, 2023
Closed
@joyeecheung joyeecheung removed the fast-track PRs that do not need to wait for 48 hours to land. label Oct 24, 2023
@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 24, 2023
@github-actions
Copy link
Contributor

Fast-track has been requested by @joyeecheung. Please 👍 to approve.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@richardlau richardlau changed the title src: fix NODE_MINOR_VERSION for v21.1.0 src: fix node_version.h Oct 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Oct 24, 2023
Closed
Merged
@nodejs-github-bot
Copy link
Collaborator

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

@richardlau richardlau mentioned this pull request Oct 24, 2023
Closed
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2023
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit 89a26b4 into nodejs:main Oct 24, 2023
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 89a26b4

@RafaelGSS
Copy link
Member

Sorry about that. As I've mentioned in the release slack channel, something might be missing in our major release guide. I've followed these strictly and something went wrong. I'll triple-check next time :)

image

@github-actions github-actions bot mentioned this pull request Oct 28, 2023
Open
22 tasks
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@joyeecheung @alexfernandez
src: fix node_version.h
eb196fd 
- Fix the version for main branch as 22.0.0
- Fix the NODE_VERSION_IS_RELEASE flag for the main branch

This would allow test/parallel/test-release-changelog.js
to pass again on the main branch.

PR-URL: nodejs#50375
Fixes: nodejs#50373
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v21.x labels Nov 11, 2023
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@richardlau richardlau richardlau approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca 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. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test/parallel/test-release-changelog.js is failing on the main branch
7 participants
@joyeecheung @richardlau @targos @nodejs-github-bot @RafaelGSS @lpinca @aduh95