New run matlab command #25
Conversation
scripts/setupdeps.sh
Outdated
| fi | ||
| mkdir -p "$WORKINGDIR/$os" | ||
| wget -O "$WORKINGDIR/$os/run-matlab-command$bin_ext" "$RMC_BASE_URL/$os/run-matlab-command$bin_ext" | ||
| mv -f ./* "$DISTDIR/" |
There was a problem hiding this comment.
Can this just be done once after the loop? Or should the specific folder we want to move be specified here?
src/matlab.ts
Outdated
| case "win32": | ||
| ext = ".exe"; | ||
| platform_dir = "win64"; | ||
| break; | ||
| case "darwin": | ||
| ext = ""; | ||
| platform_dir = "maci64"; | ||
| break; | ||
| default: | ||
| ext = ""; | ||
| platform_dir = "glnxa64"; |
There was a problem hiding this comment.
There's a bit of a disconnect here between platform and architecture. Maybe this function should take in the architecture as well so we can be sure we're picking the right binary or erroring if we don't have a binary for that combination.
There was a problem hiding this comment.
Hmmm yeah that's a good point. Is there any architectures beyond x64 that we know to be in use currently? Could this technically be a "breaking change" for some self-hosted runners?
(side note: I think it's supposed to be "easy" to cross-compile to many architectures with Golang, but I haven't tested other than x64)
There was a problem hiding this comment.
It looks like GitHub only supports x64 runners (and ARM32/64 on Linux), so we should be safe providing just x64 binaries since there is no ARM version of MATLAB yet. I think just from a code clarity and correctness standpoint, we should still take the arch into consideration here and just error if it doesn't match our expectations.
src/matlab.ts
Outdated
| */ | ||
| export function getRunMATLABCommandScriptPath(platform: string, architecture: string): string { | ||
| if (architecture != "x64") { | ||
| throw new Error(`This action is not supported for runners of platform ${platform} and architecture ${architecture}.`); |
There was a problem hiding this comment.
@mw-hrastega any thoughts on this error message?
There was a problem hiding this comment.
Could you give me an example of the message holes? Will each hole be filled with a scalar or a list?
If both are scalar, maybe something like this?
This action is not supported on ${platform} runners using the ${architecture} architecture.
There was a problem hiding this comment.
I think that sounds reasonable. Yes, they will both be scalar. One example of such an error would read This action is not supported on windows runners using the x86 architecture.
Update to use the new binary for run-matlab-command.