-
Notifications
You must be signed in to change notification settings - Fork 54
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
Binary update #250
Binary update #250
Changes from all commits
8faff92
a50c116
8f6d342
a632cfe
eba31fa
b6b23bc
f04beb9
aa4fc03
b26145f
c3f696e
7798da3
7613d21
826cd26
95fddb5
a7358f1
a839b0a
a132bff
f72c15a
436b7c6
a151840
0737671
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,14 @@ | ||
package com.mathworks.ci; | ||
/** | ||
* Copyright 2019-2020 The MathWorks, Inc. | ||
* Copyright 2019-2023 The MathWorks, Inc. | ||
* | ||
* Build Interface has two default methods. MATLAB builders can override the default behavior. | ||
* | ||
*/ | ||
|
||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.io.ByteArrayOutputStream; | ||
import org.apache.commons.lang.RandomStringUtils; | ||
import hudson.EnvVars; | ||
import hudson.FilePath; | ||
|
@@ -29,28 +30,47 @@ public interface MatlabBuild { | |
* @return matlabLauncher returns the process launcher to run MATLAB commands | ||
*/ | ||
default ProcStarter getProcessToRunMatlabCommand(FilePath workspace, | ||
Launcher launcher, TaskListener listener, EnvVars envVars, String matlabCommand, String uniqueName) | ||
Launcher launcher, TaskListener listener, EnvVars envVars, String matlabCommand, String startupOpts, String uniqueName) | ||
throws IOException, InterruptedException { | ||
// Get node specific temp .matlab directory to copy matlab runner script | ||
FilePath targetWorkspace = new FilePath(launcher.getChannel(), | ||
workspace.getRemote() + "/" + MatlabBuilderConstants.TEMP_MATLAB_FOLDER_NAME); | ||
FilePath targetWorkspace; | ||
ProcStarter matlabLauncher; | ||
if (launcher.isUnix()) { | ||
final String runnerScriptName = uniqueName + "/run_matlab_command.sh"; | ||
targetWorkspace = new FilePath(launcher.getChannel(), | ||
workspace.getRemote() + "/" + MatlabBuilderConstants.TEMP_MATLAB_FOLDER_NAME); | ||
|
||
// Determine whether we're on Mac on Linux | ||
ByteArrayOutputStream kernelStream = new ByteArrayOutputStream(); | ||
launcher.launch() | ||
.cmds("uname") | ||
.masks(true) | ||
.stdout(kernelStream) | ||
.join(); | ||
|
||
String binaryName; | ||
String runnerName = uniqueName + "/run-matlab-command"; | ||
if (kernelStream.toString("UTF-8").contains("Linux")) { | ||
binaryName = "glnxa64/run-matlab-command"; | ||
} else { | ||
binaryName = "maci64/run-matlab-command"; | ||
} | ||
|
||
matlabLauncher = launcher.launch().envs(envVars); | ||
matlabLauncher.cmds(MatlabBuilderConstants.TEMP_MATLAB_FOLDER_NAME + "/" + runnerScriptName, matlabCommand).stdout(listener); | ||
matlabLauncher.cmds(MatlabBuilderConstants.TEMP_MATLAB_FOLDER_NAME + "/" + runnerName, matlabCommand, startupOpts).stdout(listener); | ||
|
||
// Copy runner .sh for linux platform in workspace. | ||
copyFileInWorkspace(MatlabBuilderConstants.SHELL_RUNNER_SCRIPT, runnerScriptName, | ||
targetWorkspace); | ||
// Copy runner for linux platform in workspace. | ||
copyFileInWorkspace(binaryName, runnerName, targetWorkspace); | ||
} else { | ||
final String runnerScriptName = uniqueName + "\\run_matlab_command.bat"; | ||
launcher = launcher.decorateByPrefix("cmd.exe", "/C"); | ||
targetWorkspace = new FilePath(launcher.getChannel(), | ||
workspace.getRemote() + "\\" + MatlabBuilderConstants.TEMP_MATLAB_FOLDER_NAME); | ||
|
||
final String runnerName = uniqueName + "\\run-matlab-command.exe"; | ||
matlabLauncher = launcher.launch().envs(envVars); | ||
matlabLauncher.cmds(MatlabBuilderConstants.TEMP_MATLAB_FOLDER_NAME + "\\" + runnerScriptName, "\"" + matlabCommand + "\"") | ||
matlabLauncher.cmds(targetWorkspace.toString() + "\\" + runnerName, "\"" + matlabCommand + "\"", startupOpts) | ||
.stdout(listener); | ||
// Copy runner.bat for Windows platform in workspace. | ||
copyFileInWorkspace(MatlabBuilderConstants.BAT_RUNNER_SCRIPT, runnerScriptName, | ||
|
||
// Copy runner for Windows platform in workspace. | ||
copyFileInWorkspace("win64\\run-matlab-command.exe", runnerName, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally prefer static strings to be part of either properties or Constants just in cas they are changed in future, Its more manageable that way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I think that this is a good suggestion. The part that makes me hesitate here is that we're already having to hardcode in the program name on lines 54, 57, and 69. I don't really like keeping that information in two separate places, since it means if the program name were to change it would be easy for a user to just change the constant name and miss the fact that there are also hardcoded instances. I don't have a strong preference either way, it just feels a little bit odd to me to have a property in another file define one instance and a hardcoded string define another. There's also a slight readability sacrifice when future contributors have to cross reference between the file they're working on and the constants file but that's not a real issue in the long run. Also I realize I left a line of code commented out, I'll update the pull request to remove that (and also probably condense lines 54 and 57 to be one line after the if block). What are your thoughts @davidbuzinski? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong opinion unless it's being used all over the code. I think it's basically just in 1 or 2 places right? I'm fine either way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, iirc that's the only place that specific property is used |
||
targetWorkspace); | ||
} | ||
return matlabLauncher; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine, but I'm more used to special casing to mac and defaulting to Linux rather than visa versa. Not a huge deal though