Binary update #250

sameagen-MW · 2023-07-19T12:53:51Z

Updated plugin to use the run-matlab-command binary and added support for passing startup options through to MATLAB to maintain parity with cloud CI/CD platforms.

I ended up adding startupOptions as an additional argument on getProcessToRunMatlabCommand because I preferred the mental model of handling them separately, but I also think that the options could be baked into the command as well if others prefer that implementation.

Besides the small tweaks to support startup options, the primary change here is the artifacts that the pom downloads, and then some of the logic within MatlabBuild. Since there's now three option instead of two, I used a call to uname to differentiate between MacOS and Linux.

Mac Os update

src/main/resources/com/mathworks/ci/RunMatlabBuildBuilder/config.jelly

mw-hrastega · 2023-07-19T17:23:53Z

src/main/resources/com/mathworks/ci/RunMatlabBuildStep/config.jelly

"Startup Options" or "MATLAB Startup Options"?

Same question for anywhere else in this submission. I can see both "MATLAB Startup Options" and "Startup Options".

mw-hrastega · 2023-07-19T17:24:34Z

src/main/resources/com/mathworks/ci/RunMatlabBuildStep/help-startupOptions.html

@@ -0,0 +1,3 @@
+<div>
+ A space-separated list of MATLAB Startup Options.


Startup Options -> startup options

mw-hrastega · 2023-07-19T17:25:11Z

src/main/resources/com/mathworks/ci/RunMatlabCommandStep/help-startupOptions.html

@@ -0,0 +1,3 @@
+<div>
+ A space-separated list of MATLAB Startup Options.


mw-hrastega · 2023-07-19T17:25:31Z

src/main/resources/com/mathworks/ci/RunMatlabTestsStep/help-startupOptions.html

@@ -0,0 +1,3 @@
+<div>
+ A space-separated list of MATLAB Startup Options.


mw-hrastega · 2023-07-19T17:25:40Z

src/main/resources/com/mathworks/ci/StartupOptions/help-startupOptions.html

@@ -0,0 +1,3 @@
+<div>
+ A space-separated list of MATLAB Startup Options.


mw-hrastega · 2023-07-24T18:23:01Z

src/main/resources/com/mathworks/ci/RunMatlabBuildStep/config.jelly

I wonder why we use a colon in only a subset of titles. I can see both title="Startup Options: " and title="Startup Options".

Good point, I think that this was a mistake on my part. All of the builders use "Startup Options," and I tried to stick with the precedent in each of the files. For the *Steps, I thought I was going off of the precedent of "RunMatlabTestsStep" but I think I misrembered and changed "startupOptions: " into "Startup Options:". I'll get that fixed.

sameagen-MW · 2023-08-08T15:08:22Z

UI Updates

src/main/resources/com/mathworks/ci/RunMatlabTestsBuilder/config.jelly

mw-hrastega · 2023-08-08T15:21:06Z

UI Updates

Thank you Sam for including screenshots:

The check box labels elsewhere do not use title case. For example, see the labels in the Run MATLAB Tests UI: https://github.com/mathworks/jenkins-matlab-plugin/blob/master/CONFIGDOC.md#run-matlab-tests. Shouldn't Startup Options become Startup options then?
Is it possible to use a three-word heading (because the other headings are two or three words?) You can actually use this title: https://www.mathworks.com/help/matlab/matlab_env/startup-options.html. Then, you can change the check box label to "MATLAB startup options".
You are proposing a section that will have only one check box. The other sections have more than one check box. And "Source folder" doesn't belong to a section. Do you think "Startup options" should be treated like "Source folder" and placed after Source folder, or will this make the step UI inconsistent with the other two step UIs?

sameagen-MW · 2023-08-08T15:24:49Z

I don't know why but I can't respond direclty to your comment Houman. Those changes seem reasonable to me though

sameagen-MW · 2023-08-08T15:29:30Z

Updated

nbhoski

I am not sure what exactly the startup options affect the workflow and should we include it. since we have included it it also then requires Doc update with examples. @mw-hrastega do you agree?

src/main/java/com/mathworks/ci/MatlabBuild.java

nbhoski · 2023-08-22T08:21:32Z

src/main/java/com/mathworks/ci/MatlabBuild.java

            // Copy runner.bat for Windows platform in workspace.
-            copyFileInWorkspace(MatlabBuilderConstants.BAT_RUNNER_SCRIPT, runnerScriptName,
+            copyFileInWorkspace("win64\\run-matlab-command.exe", runnerName,


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

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?

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

Yeah, iirc that's the only place that specific property is used

src/main/java/com/mathworks/ci/RunMatlabCommandBuilder.java

src/main/java/com/mathworks/ci/RunMatlabTestsBuilder.java

src/main/resources/com/mathworks/ci/StartupOptions/help-startupOptions.html

src/main/java/com/mathworks/ci/RunMatlabBuildBuilder.java

sameagen-MW · 2023-08-22T13:21:54Z

I am not sure what exactly the startup options affect the workflow and should we include it. since we have included it it also then requires Doc update with examples. @mw-hrastega do you agree?

Just to double check that I understand what you mean, you would have a preference for only updating the Jenkins plugin to use run-matlab-command and not adding support for passing startup options through to MATLAB?

…ns to reduce redundancy

nbhoski · 2023-08-30T13:09:43Z

Hi @sameagen-MW coud you just confirm if we consider all CR as resolved ? I got confused seeing few unresolved comments

sameagen-MW · 2023-08-30T14:29:42Z

Hi @sameagen-MW coud you just confirm if we consider all CR as resolved ? I got confused seeing few unresolved comments

Yeah totally, I've gone and resolved all of the conversations that I think are totally finished.

davidbuzinski · 2023-08-30T14:34:57Z

src/main/java/com/mathworks/ci/MatlabBuild.java

+
+            String binaryName;
+            String runnerName = uniqueName + "/run-matlab-command";
+            if (kernelStream.toString("UTF-8").contains("Linux")) {


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

src/main/java/com/mathworks/ci/RunMatlabCommandBuilder.java

nbhoski

Ship it!

nbhoski and others added 10 commits May 23, 2023 10:39

Merge pull request #247 from mathworks/dev_main

8faff92

Mac Os update

Initial implementation pass

a50c116

Repair old tests

8f6d342

Add initial batch of unit tests for startupOptions

a632cfe

Add documentation stubs, copyrights, small tweaks to code

eba31fa

Switched UI to utilize checkbox

b6b23bc

Remove debug symbols, update clean

f04beb9

Update pipeline snippet generator templates

aa4fc03

Clean up UI, format pom.xml

b26145f

Fixed potential encoding issue

c3f696e

sameagen-MW requested review from nbhoski, mw-hrastega and davidbuzinski July 19, 2023 12:53

mw-hrastega reviewed Jul 19, 2023

View reviewed changes

src/main/resources/com/mathworks/ci/RunMatlabBuildBuilder/config.jelly Outdated Show resolved Hide resolved

mw-hrastega reviewed Jul 19, 2023

View reviewed changes

sameagen-MW added 3 commits July 19, 2023 13:46

Fix for Windows

7798da3

Fix typo

7613d21

Fix capitalization on help text

826cd26

mw-hrastega reviewed Jul 24, 2023

View reviewed changes

Change steps to use test step precedent

95fddb5

davidbuzinski assigned yitingw3 Jul 25, 2023

sameagen-MW added 2 commits August 8, 2023 11:04

Parallelize section header structure

a7358f1

Fix typo

a839b0a

mw-hrastega reviewed Aug 8, 2023

View reviewed changes

src/main/resources/com/mathworks/ci/RunMatlabTestsBuilder/config.jelly Outdated Show resolved Hide resolved

Update case

a132bff

UI updates

f72c15a

More UI tweaks

436b7c6

mw-hrastega approved these changes Aug 8, 2023

View reviewed changes

nbhoski requested changes Aug 22, 2023

View reviewed changes

Cleaned up code slightly. Updated startupOptions property to be optio…

a151840

…ns to reduce redundancy

sameagen-MW requested a review from nbhoski August 23, 2023 19:40

davidbuzinski approved these changes Aug 30, 2023

View reviewed changes

nbhoski approved these changes Aug 31, 2023

View reviewed changes

Change uniqe to unique

0737671

sameagen-MW merged commit a24d134 into dev_main Aug 31, 2023
4 checks passed

sameagen-MW deleted the BinaryUpdate branch August 31, 2023 15:02

sameagen-MW linked an issue Jun 27, 2024 that may be closed by this pull request

Allow MATLAB to run with other critical startup options #216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary update #250

Binary update #250

sameagen-MW commented Jul 19, 2023

mw-hrastega Jul 19, 2023

mw-hrastega Jul 19, 2023

mw-hrastega Jul 19, 2023

mw-hrastega Jul 19, 2023

mw-hrastega Jul 19, 2023

mw-hrastega Jul 19, 2023

mw-hrastega Jul 24, 2023

sameagen-MW Jul 24, 2023

sameagen-MW commented Aug 8, 2023

mw-hrastega commented Aug 8, 2023

UI Updates

sameagen-MW commented Aug 8, 2023

sameagen-MW commented Aug 8, 2023

nbhoski left a comment

nbhoski Aug 22, 2023

sameagen-MW Aug 22, 2023

davidbuzinski Aug 30, 2023 •

edited

Loading

sameagen-MW Aug 30, 2023 •

edited

Loading

sameagen-MW commented Aug 22, 2023

nbhoski commented Aug 30, 2023

sameagen-MW commented Aug 30, 2023

davidbuzinski Aug 30, 2023

nbhoski left a comment

		@@ -0,0 +1,3 @@
		<div>
		A space-separated list of MATLAB Startup Options.

Binary update #250

Binary update #250

Conversation

sameagen-MW commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameagen-MW commented Aug 8, 2023

UI Updates

mw-hrastega commented Aug 8, 2023

UI Updates

sameagen-MW commented Aug 8, 2023

sameagen-MW commented Aug 8, 2023

Updated

nbhoski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbuzinski Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

sameagen-MW Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

sameagen-MW commented Aug 22, 2023

nbhoski commented Aug 30, 2023

sameagen-MW commented Aug 30, 2023

Choose a reason for hiding this comment

nbhoski left a comment

Choose a reason for hiding this comment

davidbuzinski Aug 30, 2023 •

edited

Loading

sameagen-MW Aug 30, 2023 •

edited

Loading