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

jenkins: pack embedtest.exe for Windows builds #3730

Merged

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented May 17, 2024

CI runs for PR nodejs/node#52646 fail because the embedtest.exe is not packaged along with other binary files and it cannot be found by the test being enabled.

This PR changes Jenkins Windows scripts to treat embedtest.exe the same way as cctest.exe.
The only difference is that we check that embedtest.exe file exists.
It must unblock the PR nodejs/node#52646.

@richardlau
Copy link
Member

cc @StefanStojanovic

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

The changes look reasonable and good. Just to check, was this tested through CI? Eg. with a copy of the Windows job using this branch instead of the main one?

@richardlau
Copy link
Member

Just to check, was this tested through CI? Eg. with a copy of the Windows job using this branch instead of the main one?

I don't believe so.

@vmoroz
Copy link
Member Author

vmoroz commented May 20, 2024

The changes look reasonable and good. Just to check, was this tested through CI? Eg. with a copy of the Windows job using this branch instead of the main one?

No, I did not test it this way. How can I do it?

@StefanStojanovic
Copy link
Contributor

No, I did not test it this way. How can I do it?

You'd need a higher access level to Jenkins to create a job copy and point it to your branch. I will do that for you. and send the run here.

@StefanStojanovic
Copy link
Contributor

Here is the CI run (run 1 failure is something unrelated to this). I used this branch for jobs and ran the branch from nodejs/node#52646 through CI. Everything seems to be in order.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

Based on what @StefanStojanovic mentioned in terms of testing looks like this is ready to land, landing.

@mhdawson mhdawson merged commit 0169b18 into nodejs:main May 21, 2024
1 check passed
@vmoroz vmoroz deleted the pr/jenkins_scripts_windows_pack_embedtest branch May 21, 2024 17:44
@vmoroz
Copy link
Member Author

vmoroz commented May 21, 2024

Here is the CI run (run 1 failure is something unrelated to this). I used this branch for jobs and ran the branch from nodejs/node#52646 through CI. Everything seems to be in order.

Thank you, @StefanStojanovic , for verifying it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants