Better handling of node in Linux scripts. by mcottontensor · Pull Request #607 · EpicGamesExt/PixelStreamingInfrastructure

mcottontensor · 2025-05-05T01:54:37Z

Relevant components:

Problem statement:

A few issues:

The handling of node sometimes causes issues on some environments.
The semver comparison function does not work on mac.

Solution

For the node installations, we now just check whatever node is installed. If it's greater or equal to our requirements we do nothing. Otherwise we install node in our local location. If we installed node, then we update PATH to point to the new location.
If node_modules doesnt exist or package-lock is newer than the directory, we install dependencies, otherwise we assume they're up to date.
Additionally I've also added a printout of the node and npm versions used in the script.
I've removed the weird NPM variable that was used to point to the specific npm version. Since we now properly update the path it should not be needed. Also, the way we call sudo now should also allow the correct path variable to carry over to the new environment.
The version check function also now uses a simplified version utilizing 'sort -V' which is designed to sort version numbers. This should work across OSs like Linux and Mac.

Mac has issues with the way we currently check semver values. Replacing it with sort -V. This might not be available on mac but CI should tell me.

changeset-bot · 2025-05-05T01:54:40Z

⚠️ No Changeset found

Latest commit: 186cb69

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

install node deps etc.

NPM are being used.

mcottontensor · 2025-05-05T02:52:22Z

.backportrc.json

This is just piggy backing with this change. I figure it's useful to backport to LatencyTest.

lukehb · 2025-05-05T23:36:33Z

SignallingWebServer/platform_scripts/bash/common.sh

    echo "----------------------------------"

-    start_process "sudo PATH=\"$PATH\" \"$NPM\" start -- ${SERVER_ARGS}"
+    echo "sudo env \"PATH=$PATH\" npm start -- ${SERVER_ARGS}"


Is the echo here just for debug?

Yep. Will remove.

lukehb · 2025-05-05T23:38:25Z

SignallingWebServer/platform_scripts/bash/common.sh


    # setup the path so we're using our node. required when calling npm
-    PATH="${SCRIPT_DIR}/node/bin:$PATH"
+    # PATH="${SCRIPT_DIR}/node/bin:$PATH"


These comments can go (or be moved to line 177)

lukehb

Looks like a good change to me. CI is passing from what I can see. I left a few minor comments/questions - but those are not merge blockers.

Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace)

github-actions · 2025-05-06T06:26:17Z

💔 Some backports could not be created

Status	Branch	Result
❌	UE5.5	Backport failed because of merge conflicts
✅	LatencyTest

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

backport --pr 607

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

[LatencyTest] refactor: Better handling of node in Linux scripts. (#607)

mcottontensor · 2025-05-06T06:28:46Z

💚 All backports created successfully

Status	Branch	Result
✅	UE5.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace) # Conflicts: # SignallingWebServer/platform_scripts/bash/common.sh

… support (#647) * Add 4.27 support back for latency testing * Save webrtc stats to csv and auto download * Deep copy stats object * Update record parsing logic to handle missing field when generating the csv * Update session test to handle additional latency stats * Fixing path setup in the windows platform scripts. (#594) (#595) (cherry picked from commit 152abd6) Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com> (cherry picked from commit 1dedc87) * Fixing more path issues in platform_scripts (#604) * feat: Adding script tests where the scripts are run multiple times. This tests that the scripts still work when things have already been installed. * fix: Fixing typo in github action. * gimme a break. * Adding mutliple script runs for other OSs * Testing node on the runner * Trying to figure out what node is doing what. * Trying to change the way node version is detected. * Messy * The additions to the script really didn't catch anything. Could be worth revisiting how we handle the local node install. * Removing debug print. (cherry picked from commit 2321fa9) * refactor: Better handling of node in Linux scripts. (#607) Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace) * Fixing a small issue with how path was setup previously. (#611) If the node install was installed locally previously the script would not pick that up and try to install again. It would fail because the directory already exists and then the path would not get properly setup for the local install of node. This change sets PATH to point to the local install anyway. If it doesn't exist it doesn't matter, if it does exist we use the local install. (cherry picked from commit 00aaa52) * Added disable button when session test is running * Added new flag ?LatencyCSV to enable the session test and its UI * Added changeset for 4.27 support and session test feature --------- Co-authored-by: Will Belcher <william.belcher@xa.epicgames.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com>

… support (#647) * Add 4.27 support back for latency testing * Save webrtc stats to csv and auto download * Deep copy stats object * Update record parsing logic to handle missing field when generating the csv * Update session test to handle additional latency stats * Fixing path setup in the windows platform scripts. (#594) (#595) (cherry picked from commit 152abd6) Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com> (cherry picked from commit 1dedc87) * Fixing more path issues in platform_scripts (#604) * feat: Adding script tests where the scripts are run multiple times. This tests that the scripts still work when things have already been installed. * fix: Fixing typo in github action. * gimme a break. * Adding mutliple script runs for other OSs * Testing node on the runner * Trying to figure out what node is doing what. * Trying to change the way node version is detected. * Messy * The additions to the script really didn't catch anything. Could be worth revisiting how we handle the local node install. * Removing debug print. (cherry picked from commit 2321fa9) * refactor: Better handling of node in Linux scripts. (#607) Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace) * Fixing a small issue with how path was setup previously. (#611) If the node install was installed locally previously the script would not pick that up and try to install again. It would fail because the directory already exists and then the path would not get properly setup for the local install of node. This change sets PATH to point to the local install anyway. If it doesn't exist it doesn't matter, if it does exist we use the local install. (cherry picked from commit 00aaa52) * Added disable button when session test is running * Added new flag ?LatencyCSV to enable the session test and its UI * Added changeset for 4.27 support and session test feature --------- Co-authored-by: Will Belcher <william.belcher@xa.epicgames.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com> (cherry picked from commit c61fd7b)

… support (#647) (#651) * Add 4.27 support back for latency testing * Save webrtc stats to csv and auto download * Deep copy stats object * Update record parsing logic to handle missing field when generating the csv * Update session test to handle additional latency stats * Fixing path setup in the windows platform scripts. (#594) (#595) (cherry picked from commit 152abd6) (cherry picked from commit 1dedc87) * Fixing more path issues in platform_scripts (#604) * feat: Adding script tests where the scripts are run multiple times. This tests that the scripts still work when things have already been installed. * fix: Fixing typo in github action. * gimme a break. * Adding mutliple script runs for other OSs * Testing node on the runner * Trying to figure out what node is doing what. * Trying to change the way node version is detected. * Messy * The additions to the script really didn't catch anything. Could be worth revisiting how we handle the local node install. * Removing debug print. (cherry picked from commit 2321fa9) * refactor: Better handling of node in Linux scripts. (#607) Updated the way the bash script sets up the environment for node. Node will only be installed if a version meeting the minimum requirements is not found. The new environment should result in a more robust environment setup. (cherry picked from commit f27bace) * Fixing a small issue with how path was setup previously. (#611) If the node install was installed locally previously the script would not pick that up and try to install again. It would fail because the directory already exists and then the path would not get properly setup for the local install of node. This change sets PATH to point to the local install anyway. If it doesn't exist it doesn't matter, if it does exist we use the local install. (cherry picked from commit 00aaa52) * Added disable button when session test is running * Added new flag ?LatencyCSV to enable the session test and its UI * Added changeset for 4.27 support and session test feature --------- (cherry picked from commit c61fd7b) Co-authored-by: Luke Bermingham <1215582+lukehb@users.noreply.github.com> Co-authored-by: Will Belcher <william.belcher@xa.epicgames.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com>

Updating the platform scripts again.

0926b65

Mac has issues with the way we currently check semver values. Replacing it with sort -V. This might not be available on mac but CI should tell me.

mcottontensor added 6 commits May 5, 2025 11:56

Use npm from path not a direct path.

772e4f2

Installing node only if we need to. Tweaking path usage and when to

f889faa

install node deps etc.

Adding some auditing logs so we can confirm what versions of node and

7462825

NPM are being used.

adding a debug test for mac

3448c01

More testing for mac.

d5c31d8

Removing temp debug test

820ea5d

mcottontensor changed the title ~~Updating the platform scripts again.~~ Better handling of node in Linux scripts. May 5, 2025

mcottontensor marked this pull request as ready for review May 5, 2025 02:47

mcottontensor added auto-backport Used to specify we want a PR to auto backport to a branch, must be paired with auto-backport-to-UEX. auto-backport-to-UE5.5 auto-backport-to-LatencyTest labels May 5, 2025

mcottontensor commented May 5, 2025

View reviewed changes

lukehb reviewed May 5, 2025

View reviewed changes

Removing some debug prints and comments.

186cb69

lukehb approved these changes May 5, 2025

View reviewed changes

mcottontensor merged commit f27bace into EpicGamesExt:master May 6, 2025
8 checks passed

mcottontensor deleted the script_update_for_mac branch May 6, 2025 06:26

github-actions bot mentioned this pull request May 6, 2025

[LatencyTest] refactor: Better handling of node in Linux scripts. (#607) #609

Merged

github-actions bot added a commit that referenced this pull request May 6, 2025

Merge pull request #609 from EpicGamesExt/backport/LatencyTest/pr-607

4dd8c0e

[LatencyTest] refactor: Better handling of node in Linux scripts. (#607)

mcottontensor mentioned this pull request May 6, 2025

[UE5.5] refactor: Better handling of node in Linux scripts. (#607) #610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handling of node in Linux scripts.#607

Better handling of node in Linux scripts.#607
mcottontensor merged 8 commits intoEpicGamesExt:masterfrom
mcottontensor:script_update_for_mac

mcottontensor commented May 5, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented May 5, 2025 •

edited

Loading

Uh oh!

mcottontensor May 5, 2025

Uh oh!

lukehb May 5, 2025

Uh oh!

mcottontensor May 5, 2025

Uh oh!

lukehb May 5, 2025

Uh oh!

lukehb left a comment

Uh oh!

Uh oh!

github-actions bot commented May 6, 2025

Uh oh!

mcottontensor commented May 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mcottontensor commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Relevant components:

Problem statement:

Solution

Uh oh!

changeset-bot bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

mcottontensor May 5, 2025

Choose a reason for hiding this comment

Uh oh!

lukehb May 5, 2025

Choose a reason for hiding this comment

Uh oh!

mcottontensor May 5, 2025

Choose a reason for hiding this comment

Uh oh!

lukehb May 5, 2025

Choose a reason for hiding this comment

Uh oh!

lukehb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 6, 2025

💔 Some backports could not be created

Manual backport

Questions ?

Uh oh!

mcottontensor commented May 6, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mcottontensor commented May 5, 2025 •

edited

Loading

changeset-bot bot commented May 5, 2025 •

edited

Loading