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

lib / src : add undici version to process.versions #45599

Closed

Conversation

kivancbilen
Copy link

@kivancbilen kivancbilen commented Nov 23, 2022

process.versions is not returning undici version,
this PR will read the version number from undici/src/package.json and add it to result. If this approach is ok then other missing libraries will be added

Refs: #45260

@panva
Copy link
Member

panva commented Nov 23, 2022

👋 and thank you for your contribution!

Please update the commit message to use Refs instead of Fixes on the #45260 reference as it does not completely resolve it.

while (!hasEnding(cwd.parent_path().string(), "node")) {
cwd = cwd.parent_path();
}
std::ifstream myFile(cwd.parent_path().string()+"\\deps\\" + packageName + "\\src\\package.json");
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this is not going to work. This file won't exist for the binaries available from nodejs.org.

Copy link
Member

Choose a reason for hiding this comment

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

@richardlau How about updating the update script for undici to statically update a C++ variable with the latest version number?

Copy link
Author

Choose a reason for hiding this comment

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

Would this also be an option, read versions and write them in a text file in build step, then read that file to return missing versions? File and node.exe can be in the same directory.

Copy link
Member

@richardlau richardlau Nov 24, 2022

Choose a reason for hiding this comment

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

Node.js is compiled into a single binary so you cannot assume any other files are present at run time.

Copy link
Member

Choose a reason for hiding this comment

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

You can create a dep-versions.cc and replace the value on tools/deps_updater scripts either through bash or javascript.

process.versions is not returning undici version,
this PR will read the version number from undici/src/package.json
and add it to result. If this approach is ok then other missing libraries
will be added

Refs:nodejs#45260
debadree25 added a commit to debadree25/node that referenced this pull request Nov 25, 2022
Attempted to update undici version when the update scripts are run
Fixes: nodejs#45260
Refs: nodejs#45599
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Really appreciate you taking this on, but this is definitely the wrong approach. The versions should be extracted either when the dependency is updated or the binary is built.

@kivancbilen
Copy link
Author

#45621 is already approved, therefore I am closing this PR.

debadree25 added a commit to debadree25/node that referenced this pull request Dec 12, 2022
Attempted to update undici version when the update scripts are run
Fixes: nodejs#45260
Refs: nodejs#45599
aduh95 pushed a commit that referenced this pull request Dec 18, 2022
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>
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 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants