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

Improve names of .node files #768

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Improve names of .node files #768

wants to merge 14 commits into from

Conversation

Bashamega
Copy link

fix #701

Tyriar
Tyriar requested changes Mar 14, 2025
View reviewed changes
binding.gyp Outdated
@@ -32,7 +32,7 @@
{
'target_name': 'conpty',
'sources' : [
'src/win/conpty.cc',
'src/win/conpty_backend.cc',
Copy link
Member

Choose a reason for hiding this comment

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

Is it target_name we need to change as well to change the .node file, then an update here is needed too:

node-pty/src/windowsPtyAgent.ts

Lines 64 to 93 in 467cea0

if (!conptyNative) {
try {
conptyNative = require('../build/Release/conpty.node');
} catch (outerError) {
try {
conptyNative = require('../build/Debug/conpty.node');
} catch (innerError) {
console.error('innerError', innerError);
// Re-throw the exception from the Release require if the Debug require fails as well
throw outerError;
}
}
}
} else {
if (!winptyNative) {
try {
winptyNative = require('../build/Release/pty.node');
} catch (outerError) {
try {
winptyNative = require('../build/Debug/pty.node');
} catch (innerError) {
console.error('innerError', innerError);
// Re-throw the exception from the Release require if the Debug require fails as well
throw outerError;
}
}
}
}
this._ptyNative = this._useConpty ? conptyNative : winptyNative;

Copy link
Author

Choose a reason for hiding this comment

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

Thank you. I will update it

Copy link
Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Member

Choose a reason for hiding this comment

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

We just need to figure out why CI is failing now. See the error at https://dev.azure.com/vscode/node-pty/_build/results?buildId=147363&view=logs&j=0bc77094-9fcd-5c38-f6e4-27d2ae131589&t=2917e7a9-bb9d-585b-890e-1ad69b6f3f65&l=976

Copy link
Contributor

Choose a reason for hiding this comment

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

Name should be updated in

node-pty/scripts/post-install.js

Lines 10 to 11 in 090384f

path.join(RELEASE_DIR, 'conpty.node'),
path.join(RELEASE_DIR, 'conpty.pdb'),
to fix the build error.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you

Copy link
Author

Choose a reason for hiding this comment

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

Hello @deepak1556
I have updated the post-install script, but it is still not working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check #768 (comment)

@Bashamega
Copy link
Author

Hello @Tyriar
I don't know why it is not working, it is working on Linux and Mac.
I don't know c++ or how this project is built.
Can you help me?
Thanks.

@Tyriar
Copy link
Member

Tyriar commented Mar 14, 2025

That error means that the expected compile .node file isn't there, so the change in binding.gyp didn't seem to do the right file or something. I don't have time to look into this further atm unfortunately.

@Bashamega
Copy link
Author

That error means that the expected compile .node file isn't there, so the change in binding.gyp didn't seem to do the right file or something. I don't have time to look into this further atm unfortunately.

I will try to look at it more

deepak1556
deepak1556 reviewed Mar 15, 2025
View reviewed changes
binding.gyp Outdated
'sources' : [
'src/win/conpty.cc',
'src/win/conpty_backend.cc',
Copy link
Contributor

Choose a reason for hiding this comment

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

changing the target name is sufficient to achieve the required result, lets keep the source file names unchanged.

Copy link
Author

Choose a reason for hiding this comment

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

Okay

deepak1556
deepak1556 reviewed Mar 15, 2025
View reviewed changes
binding.gyp Outdated
@@ -58,7 +58,7 @@
'deps/winpty/src/winpty.gyp:winpty',
],
'sources' : [
'src/win/winpty.cc',
'src/win/winpty_backend.cc',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required for the scope of this PR, lets revert this.

deepak1556
deepak1556 reviewed Mar 17, 2025
View reviewed changes
scripts/post-install.js Outdated
Comment on lines 69 to 71
const destFolder = path.join(RELEASE_DIR, 'conpty');
const destFolder = path.join(RELEASE_DIR, 'conpty_backend');
fs.mkdirSync(destFolder, { recursive: true });
for (const file of ['conpty.dll', 'OpenConsole.exe']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These should not be renamed, they are prebuilt binaries from conpty library.

@deepak1556
Copy link
Contributor

There is one another place to update

node-pty/src/win/conpty.cc

Line 171 in bfbd943

HMODULE hModule = GetModuleHandleA("conpty.node");
and that should fix the failing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepak1556 deepak1556 deepak1556 left review comments

@Tyriar Tyriar Tyriar requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@deepak1556 deepak1556

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve names of .node files
3 participants
@Bashamega @Tyriar @deepak1556