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

Prefer node16 over node12 when running internal scripts #1621

Merged
merged 28 commits into from
Feb 14, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
af424b9
Use 16 to run RunnerService.js
fhammerl Jan 20, 2022
cd22b4e
Execute hashfiles using node16
fhammerl Jan 20, 2022
aaeecde
Run downloadCert.js using node16
fhammerl Jan 21, 2022
194bc7c
Run makeWebRequest.js using node16
fhammerl Jan 21, 2022
5ce726c
Run macos-run-invoker.js using node16
fhammerl Jan 21, 2022
0326552
Run hashFiles.js using node16
fhammerl Jan 21, 2022
bbca7b9
Update tests to use node16
fhammerl Jan 21, 2022
22b07a3
Update documentation to recommend node16
fhammerl Jan 21, 2022
c1433ee
Duplicate macos service js fix for 16
fhammerl Jan 21, 2022
5b2b6b5
Add PR link
fhammerl Jan 24, 2022
d88bd65
Revert ADR node change
fhammerl Jan 24, 2022
e6e313e
Merge node12/16 retainment IFs
fhammerl Jan 24, 2022
155ca9e
Try both node12 and node16
fhammerl Jan 24, 2022
8940886
Close if
fhammerl Jan 26, 2022
76b2442
Revert "Update tests to use node16"
fhammerl Jan 26, 2022
98627f8
Fix condition
fhammerl Jan 26, 2022
cc31a62
Unfurl if condition
fhammerl Feb 2, 2022
90300da
Allow user to force a node version
fhammerl Feb 2, 2022
df8aa7c
Format update template
fhammerl Feb 2, 2022
a5f9652
Comment env var
fhammerl Feb 2, 2022
e99b8d0
Rename vars
fhammerl Feb 7, 2022
0117291
Fix naming
fhammerl Feb 7, 2022
68ac419
Fix rename
Feb 8, 2022
4a2b976
Merge branch 'main' of https://github.com/actions/runner into fhammer…
fhammerl Feb 10, 2022
cc78578
Set node ver override if job message has it
fhammerl Feb 10, 2022
93e5fe0
Merge branch 'fhammerl/prefer-node16' of https://github.com/actions/r…
fhammerl Feb 10, 2022
3562a88
Format executionContext
fhammerl Feb 10, 2022
df7fb5b
Can only receive 'forceNode12' or nothing from FF
fhammerl Feb 11, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/checks/nodejs.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@

Make sure the built-in node.js has access to GitHub.com or GitHub Enterprise Server.

The runner carries it's own copy of node.js executable under `<runner_root>/externals/node12/`.
The runner carries its own copy of node.js executable under `<runner_root>/externals/node16/`.

All javascript base Actions will get executed by the built-in `node` at `<runner_root>/externals/node12/`.
All javascript base Actions will get executed by the built-in `node` at `<runner_root>/externals/node16/`.

> Not the `node` from `$PATH`

Expand Down
5 changes: 3 additions & 2 deletions src/Misc/layoutbin/runsvc.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ if [ -f ".path" ]; then
echo ".path=${PATH}"
fi

# insert anything to setup env when running as a service
nodever=${GITHUB_ACTIONS_RUNNER_FORCED_NODE_VERSION:-node16}
fhammerl marked this conversation as resolved.
Show resolved Hide resolved

# insert anything to setup env when running as a service
# run the host process which keep the listener alive
./externals/node12/bin/node ./bin/RunnerService.js &
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
./externals/$nodever/bin/node ./bin/RunnerService.js &
PID=$!
wait $PID
trap - TERM INT
Expand Down
13 changes: 10 additions & 3 deletions src/Misc/layoutbin/update.sh.template
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,13 @@ then
exit 1
fi


# fix upgrade issue with macOS when running as a service
attemptedtargetedfix=0
currentplatform=$(uname | awk '{print tolower($0)}')
if [[ "$currentplatform" == 'darwin' && restartinteractiverunner -eq 0 ]]; then
# We needed a fix for https://github.com/actions/runner/issues/743
# We will recreate the ./externals/node12/bin/node of the past runner version that launched the runnerlistener service
# We will recreate the ./externals/nodeXY/bin/node of the past runner version that launched the runnerlistener service
# Otherwise mac gatekeeper kills the processes we spawn on creation as we are running a process with no backing file

# We need the pid for the nodejs loop, get that here, its the parent of the runner C# pid
Expand All @@ -135,7 +136,13 @@ if [[ "$currentplatform" == 'darwin' && restartinteractiverunner -eq 0 ]]; then
then
# inspect the open file handles to find the node process
# we can't actually inspect the process using ps because it uses relative paths and doesn't follow symlinks
path=$(lsof -a -g "$procgroup" -F n | grep node12/bin/node | grep externals | tail -1 | cut -c2-)
nodever="node16"
path=$(lsof -a -g "$procgroup" -F n | grep $nodever/bin/node | grep externals | tail -1 | cut -c2-)
if [[ $? -ne 0 || -z "$path" ]] # Fallback if RunnerService.js was started with node12
then
nodever="node12"
path=$(lsof -a -g "$procgroup" -F n | grep $nodever/bin/node | grep externals | tail -1 | cut -c2-)
fi
if [[ $? -eq 0 && -n "$path" ]]
then
# trim the last 5 characters of the path '/node'
Expand All @@ -148,7 +155,7 @@ if [[ "$currentplatform" == 'darwin' && restartinteractiverunner -eq 0 ]]; then
then
date "+[%F %T-%4N] Creating fallback node at path $path" >> "$logfile" 2>&1
mkdir -p "$trimmedpath"
cp "$rootfolder/externals/node12/bin/node" "$path"
cp "$rootfolder/externals/$nodever/bin/node" "$path"
else
date "+[%F %T-%4N] Path for fallback node exists, skipping creating $path" >> "$logfile" 2>&1
fi
Expand Down
1 change: 1 addition & 0 deletions src/Runner.Common/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ public static class Actions
public static class Agent
{
public static readonly string ToolsDirectory = "agent.ToolsDirectory";
public static readonly string ForcedNodeVersion = "GITHUB_ACTIONS_RUNNER_FORCED_NODE_VERSION";
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
}

public static class System
Expand Down
16 changes: 16 additions & 0 deletions src/Runner.Common/Util/NodeUtil.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
namespace GitHub.Runner.Common.Util
{
using System;
using System.Collections.ObjectModel;
fhammerl marked this conversation as resolved.
Show resolved Hide resolved

fhammerl marked this conversation as resolved.
Show resolved Hide resolved
public static class NodeUtil
{
public const string LatestNodeVersion = "node16";
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
public static readonly ReadOnlyCollection<string> AllowedNodeVersions = new(new[] {"node12", "node16"});
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
public static string GetForcedOrLatestNodeVersion()
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
{
var forcedNodeVersion = Environment.GetEnvironmentVariable(Constants.Variables.Agent.ForcedNodeVersion);
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
fhammerl marked this conversation as resolved.
Show resolved Hide resolved
return string.IsNullOrEmpty(forcedNodeVersion) ? LatestNodeVersion : forcedNodeVersion;
}
}
}
7 changes: 4 additions & 3 deletions src/Runner.Listener/Checks/CheckUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Threading;
using System.Threading.Tasks;
using GitHub.Runner.Common;
using GitHub.Runner.Common.Util;
using GitHub.Runner.Sdk;
using GitHub.Services.Common;

Expand Down Expand Up @@ -314,12 +315,12 @@ public static async Task<CheckResult> DownloadExtraCA(this IHostContext hostCont
});

var downloadCertScript = Path.Combine(hostContext.GetDirectory(WellKnownDirectory.Bin), "checkScripts", "downloadCert");
var node12 = Path.Combine(hostContext.GetDirectory(WellKnownDirectory.Externals), "node12", "bin", $"node{IOUtil.ExeExtension}");
result.Logs.Add($"{DateTime.UtcNow.ToString("O")} Run '{node12} \"{downloadCertScript}\"' ");
var node = Path.Combine(hostContext.GetDirectory(WellKnownDirectory.Externals), NodeUtil.GetForcedOrLatestNodeVersion(), "bin", $"node{IOUtil.ExeExtension}");
result.Logs.Add($"{DateTime.UtcNow.ToString("O")} Run '{node} \"{downloadCertScript}\"' ");
result.Logs.Add($"{DateTime.UtcNow.ToString("O")} {StringUtil.ConvertToJson(env)}");
await processInvoker.ExecuteAsync(
hostContext.GetDirectory(WellKnownDirectory.Root),
node12,
node,
$"\"{downloadCertScript}\"",
env,
true,
Expand Down
7 changes: 4 additions & 3 deletions src/Runner.Listener/Checks/NodeJsCheck.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Threading;
using System.Threading.Tasks;
using GitHub.Runner.Common;
using GitHub.Runner.Common.Util;
using GitHub.Runner.Sdk;

namespace GitHub.Runner.Listener.Check
Expand Down Expand Up @@ -144,12 +145,12 @@ private async Task<CheckResult> CheckNodeJs(string url, string pat, bool extraCA
});

var makeWebRequestScript = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Bin), "checkScripts", "makeWebRequest.js");
var node12 = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Externals), "node12", "bin", $"node{IOUtil.ExeExtension}");
result.Logs.Add($"{DateTime.UtcNow.ToString("O")} Run '{node12} \"{makeWebRequestScript}\"' ");
var node = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Externals), NodeUtil.GetForcedOrLatestNodeVersion(), "bin", $"node{IOUtil.ExeExtension}");
result.Logs.Add($"{DateTime.UtcNow.ToString("O")} Run '{node} \"{makeWebRequestScript}\"' ");
result.Logs.Add($"{DateTime.UtcNow.ToString("O")} {StringUtil.ConvertToJson(env)}");
await processInvoker.ExecuteAsync(
HostContext.GetDirectory(WellKnownDirectory.Root),
node12,
node,
$"\"{makeWebRequestScript}\"",
env,
true,
Expand Down
5 changes: 3 additions & 2 deletions src/Runner.Listener/SelfUpdater.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Threading.Tasks;
using GitHub.DistributedTask.WebApi;
using GitHub.Runner.Common;
using GitHub.Runner.Common.Util;
using GitHub.Runner.Sdk;
using GitHub.Services.Common;
using GitHub.Services.WebApi;
Expand Down Expand Up @@ -761,7 +762,7 @@ private async Task<bool> RestoreTrimmedExternals(string downloadDirectory, Cance
IOUtil.CopyDirectory(_externalsCloneDirectory, Path.Combine(downloadDirectory, Constants.Path.ExternalsDirectory), token);

// try run node.js to see if current node.js works fine after copy over to new location.
var nodeVersions = new[] { "node12", "node16" };
var nodeVersions = NodeUtil.AllowedNodeVersions;
foreach (var nodeVersion in nodeVersions)
{
var newNodeBinary = Path.Combine(downloadDirectory, Constants.Path.ExternalsDirectory, nodeVersion, "bin", $"node{IOUtil.ExeExtension}");
Expand Down Expand Up @@ -1026,7 +1027,7 @@ private async Task<string> HashFiles(string fileFolder, CancellationToken token)

var stopWatch = Stopwatch.StartNew();
string binDir = HostContext.GetDirectory(WellKnownDirectory.Bin);
string node = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Externals), "node12", "bin", $"node{IOUtil.ExeExtension}");
string node = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Externals), NodeUtil.GetForcedOrLatestNodeVersion(), "bin", $"node{IOUtil.ExeExtension}");
string hashFilesScript = Path.Combine(binDir, "hashFiles");
var hashResult = string.Empty;

Expand Down
3 changes: 2 additions & 1 deletion src/Runner.Worker/Expressions/HashFilesFunction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Reflection;
using System.Threading;
using System.Collections.Generic;
using GitHub.Runner.Common.Util;

namespace GitHub.Runner.Worker.Expressions
{
Expand Down Expand Up @@ -62,7 +63,7 @@ protected sealed override Object EvaluateCore(
string binDir = Path.GetDirectoryName(Assembly.GetEntryAssembly().Location);
string runnerRoot = new DirectoryInfo(binDir).Parent.FullName;

string node = Path.Combine(runnerRoot, "externals", "node12", "bin", $"node{IOUtil.ExeExtension}");
string node = Path.Combine(runnerRoot, "externals", NodeUtil.GetForcedOrLatestNodeVersion(), "bin", $"node{IOUtil.ExeExtension}");
string hashFilesScript = Path.Combine(binDir, "hashFiles");
var hashResult = string.Empty;
var p = new ProcessInvoker(new HashFilesTrace(context.Trace));
Expand Down
5 changes: 3 additions & 2 deletions src/Runner.Worker/Handlers/ScriptHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Linq;
using GitHub.DistributedTask.Pipelines.ContextData;
using GitHub.Runner.Common;
using GitHub.Runner.Common.Util;
using GitHub.Runner.Sdk;
using GitHub.DistributedTask.WebApi;
using Pipelines = GitHub.DistributedTask.Pipelines;
Expand Down Expand Up @@ -271,10 +272,10 @@ public async Task RunAsync(ActionRunStage stage)
if (Environment.ContainsKey("DYLD_INSERT_LIBRARIES")) // We don't check `isContainerStepHost` because we don't support container on macOS
{
// launch `node macOSRunInvoker.js shell args` instead of `shell args` to avoid macOS SIP remove `DYLD_INSERT_LIBRARIES` when launch process
string node12 = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Externals), "node12", "bin", $"node{IOUtil.ExeExtension}");
string node = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Externals), NodeUtil.GetForcedOrLatestNodeVersion(), "bin", $"node{IOUtil.ExeExtension}");
string macOSRunInvoker = Path.Combine(HostContext.GetDirectory(WellKnownDirectory.Bin), "macos-run-invoker.js");
arguments = $"\"{macOSRunInvoker.Replace("\"", "\\\"")}\" \"{fileName.Replace("\"", "\\\"")}\" {arguments}";
fileName = node12;
fileName = node;
}
#endif
var systemConnection = ExecutionContext.Global.Endpoints.Single(x => string.Equals(x.Name, WellKnownServiceEndpointNames.SystemVssConnection, StringComparison.OrdinalIgnoreCase));
Expand Down