Update ParallelizableBuildSummaryPlugin.m to fix Windows filepath issue.#3
Update ParallelizableBuildSummaryPlugin.m to fix Windows filepath issue.#3sameagen-MW wants to merge 8 commits intomainfrom
Conversation
mcafaro
left a comment
mcafaro
left a comment
There was a problem hiding this comment.
Should we put in a regression test?
| try | ||
| save(name, "taskDetail"); | ||
| catch e | ||
| warning("buildframework:BuildSummaryPlugin:UnableToSaveTrace", "Unable to save an artifact required to create the MATLAB build summary table"); |
There was a problem hiding this comment.
Not directly related to this review but these error/warning ids and package names are not currently consistent with our guidelines. The first component should relate to the product containing the code (I believe we use "ciplugins" on Jenkins). I opened an issue to track this: #4
| runTask@matlab.buildtool.plugins.BuildRunnerPlugin(plugin, pluginData); | ||
|
|
||
| name = fullfile(plugin.TempFolder, pluginData.Name + ".mat"); | ||
| name = fullfile(plugin.TempFolder, matlab.lang.internal.uuid() + ".mat"); |
There was a problem hiding this comment.
We should try to avoid using internal packages, especially in code outside of the base product like this. What about using matlab.lang.makeValidName to turn the task name into a valid identifier, which should also be a valid filename?
Yeah for sure. One of the downsides of this repository structure though is I don't think right now we have it set up so we can actually run that here. We'll probably need to test it in the |
I'll set this up in a separate PR to the |
Opened #5 |
mcafaro
left a comment
mcafaro
left a comment
There was a problem hiding this comment.
Looks good. Thanks for adding some tests!
| arguments | ||
| options.TempFolder (1,:) string = string.empty() | ||
| end | ||
|
|
||
| if ~isempty(options.TempFolder) | ||
| tempRoot = options.TempFolder; | ||
| else | ||
| tempRoot = getenv("RUNNER_TEMP"); | ||
| end |
There was a problem hiding this comment.
| arguments | |
| options.TempFolder (1,:) string = string.empty() | |
| end | |
| if ~isempty(options.TempFolder) | |
| tempRoot = options.TempFolder; | |
| else | |
| tempRoot = getenv("RUNNER_TEMP"); | |
| end | |
| arguments | |
| options.TempFolder (1,1) string = getenv("RUNNER_TEMP") | |
| end |
| - uses: actions/checkout@v5 | ||
| - uses: matlab-actions/setup-matlab@v2 | ||
| with: | ||
| release: latest-including-prerelease |
There was a problem hiding this comment.
Should we test against the oldest and newest releases of MATLAB we claim to support?
There was a problem hiding this comment.
This specific plugin is only used for R2026a+, so it's not relevant here. As we add tests for the other plugins I think that makes sense though.
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - os: ubuntu-latest | ||
| - os: windows-latest | ||
| - os: macos-latest | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - uses: matlab-actions/setup-matlab@v2 | ||
| with: | ||
| release: latest-including-prerelease | ||
| - uses: matlab-actions/run-tests@v2 |
There was a problem hiding this comment.
Nitpick: I think for the rest of the file, we have 2 spaces for indent. It would be nice to be consistent here as well
4 participants
Currently the plugin uses the task name to determine the name of the file to save the task detail in. For example, a task
t1will have it's detail saved ast1.mat.This causes issues with tasks that are part of a task group on Windows, since
:is not a valid character that can be used for file names. For examplemex:myfile.matis not a valid file name.This change switches over to using UUIDs instead, and also adds a bit of robustness so failing to save a detail will throw a warning but not stop the build.