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.

Sign up for GitHub

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

Fixes to build MATLAB toolboxes on CI #3521

Merged
merged 3 commits into from
Feb 10, 2025
Merged

Fixes to build MATLAB toolboxes on CI #3521

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7deb98
Fixes to build MATLAB toolboxes on CI
pepone Feb 7, 2025
5eae6d7
Fix MATLAB command
pepone Feb 8, 2025
e8e85e1
Review fixes
pepone Feb 10, 2025
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
2 changes: 1 addition & 1 deletion .github/actions/setup-matlab/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,4 @@ runs:
Invoke-WebRequest https://ssd.mathworks.com/supportfiles/ci/run-matlab-command/v2/win64/run-matlab-command.exe -OutFile C:\Windows\System32\run-matlab-command.exe
echo "MATLAB_COMMAND=C:\Windows\System32\run-matlab-command.exe" >> $env:GITHUB_ENV
shell: powershell
if: runner.os == 'Windows' && matrix.config == 'matlab'
if: runner.os == 'Windows'
4 changes: 3 additions & 1 deletion .github/workflows/build-matlab-packages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ jobs:
use_matlab: true

- name: Build MATLAB ToolBox
run: make -C matlab toolbox
run: |
make -C cpp srcs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were missing the C++ dependencies here

make -C matlab toolbox
if: runner.os == 'Linux'

- name: Build MATLAB ToolBox
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,6 @@ jobs:
with:
working_directory: ${{ matrix.working_directory || '.' }}
flags: ${{ matrix.test_flags }}
# Don't test matlab on Windows (see setup-dependencies/action.yml)
if: matrix.config != 'matlab' || runner.os != 'Windows'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MATLAB For Windows should work with the fixes to Windows builds.


- name: Cross Test ${{ matrix.config }} on ${{ matrix.os }}
uses: ./.github/actions/test
Expand Down
6 changes: 6 additions & 0 deletions matlab/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,15 @@ $(icetoolbox_file):: $(icethunk_target) $(slice2matlab_path) $(lang_srcdir)/lib/
cp -rf $(cpp_bindir)/slice2matlab $(lang_srcdir)/toolbox/build/
# Doc files
cp -rf $(lang_srcdir)/toolbox/doc $(lang_srcdir)/toolbox/build
ifneq (,$(MATLAB_COMMAND))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MATLAB_COMMAND is set by CI to the executable that we need to run. We were already using this in the tests, but there were a few additional places where we still used matlab directly.

cd $(lang_srcdir)/toolbox && $(MATLAB_COMMAND) "addFolderToPath '`realpath build`'"
cd $(lang_srcdir)/toolbox && $(MATLAB_COMMAND) "buildToolbox '$(version)'"
cd $(lang_srcdir)/toolbox && $(MATLAB_COMMAND) "removeFolderFromPath '`realpath build`'"
else
cd $(lang_srcdir)/toolbox && $(MATLAB_HOME)/bin/matlab -nodisplay -r "addFolderToPath '`realpath build`'"
cd $(lang_srcdir)/toolbox && $(MATLAB_HOME)/bin/matlab -nodisplay -r "buildToolbox '$(version)'"
cd $(lang_srcdir)/toolbox && $(MATLAB_HOME)/bin/matlab -nodisplay -r "removeFolderFromPath '`realpath build`'"
endif

clean::
rm -rf $(icetoolbox_file)
Expand Down
54 changes: 24 additions & 30 deletions matlab/msbuild/ice.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,36 +6,30 @@
<Configuration Condition="'$(Configuration)' == ''">Release</Configuration>
<RootDir>$([System.IO.Path]::GetFullPath( $(MSBuildThisFileDirectory)..\msbuild ))</RootDir>
<ToolboxDir>$(MSBuildThisFileDirectory)..\toolbox\build</ToolboxDir>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This MSBuild project was trying to guess the version and the matlab home using the matlab executable from path. It was far too complicated.

I simplified to use a default and allow override with env variables. It was the usage of matlab here what was causing the CI issues.

<MatlabVersion Condition="'$(MatlabVersion)' == ''">R2024a</MatlabVersion>
<MatlabHome Condition="'$(MatlabHome)' == ''">C:\Program Files\MATLAB\$(MatlabVersion)</MatlabHome>
<!-- Override with the env settings -->
<MatlabVersion Condition="'$(MATLAB_VERSION)' != ''">$(MATLAB_VERSION)</MatlabVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this go up on line 10?

Presumably if you set MATLAB_VERSION but not MATLAB_HOME, you want MatlabHome to be set correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Fixed

<MatlabHome Condition="'$(MATLAB_HOME)' != ''">$(MATLAB_HOME)</MatlabHome>
</PropertyGroup>

<Choose>
<When Condition="'$(MATLAB_COMMAND)' != ''">
<PropertyGroup>
<MatlabCommand>$(MATLAB_COMMAND)</MatlabCommand>
</PropertyGroup>
</When>
<Otherwise>
<PropertyGroup>
<MatlabCommand>&quot;$(MatlabHome)\bin\matlab.exe&quot; -nodesktop -nosplash -wait -log -minimize</MatlabCommand>
</PropertyGroup>
</Otherwise>
</Choose>

<Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" />
<Import Project="$(MSBuildThisFileDirectory)..\..\config\icebuilder.props"/>
<Import Project="$(MSBuildThisFileDirectory)..\..\config\ice.common.targets"/>

<Target Name="MatlabHome" Condition="'$(MatlabHome)' == ''">
<Exec Command="where matlab" ConsoleToMSBuild="true" EchoOff="yes">
<Output TaskParameter="ConsoleOutput" PropertyName="MatlabExe" />
</Exec>
<PropertyGroup>
<MatlabExe>$(MatlabExe.Split(';')[0])</MatlabExe>
</PropertyGroup>
<Error Text="Cannot detect a valid MATLAB installation" Condition="!Exists('$(MatlabExe)')"/>
<CreateProperty Value="$([System.IO.Path]::GetFullPath( '$(MatlabExe)\..\..' ))">
<Output TaskParameter="Value" PropertyName="MatlabHome" />
</CreateProperty>
</Target>

<Target Name="MatlabVersion" Condition="'$(MatlabVersion)' == ''" DependsOnTargets="MatlabHome">
<Exec Command="&quot;$(MatlabHome)\bin\matlab.exe&quot; -nodesktop -nosplash -wait -log -minimize -r &quot;fprintf(2, '%%s', version('-release'));exit(0);&quot;"
EchoOff="yes"
ConsoleToMSBuild="true">
<Output TaskParameter="ConsoleOutput" PropertyName="MatlabOut"/>
</Exec>
<CreateProperty Value="$(MatlabOut.Substring($(MatlabOut.LastIndexOf(';'))).Replace(';',''))">
<Output TaskParameter="Value" PropertyName="MatlabVersion" />
</CreateProperty>
</Target>

<Target Name="NuGetRestore" DependsOnTargets="GetNuGet">
<Exec Command="$(NuGetExe) restore $(MSBuildThisFileDirectory)..\..\cpp\msbuild\ice.sln" />
</Target>
Expand All @@ -47,7 +41,7 @@
Properties="Platform=$(Platform);Configuration=$(Configuration)" />
</Target>

<Target Name="BuildDist" DependsOnTargets="BuildCppDist;NuGetRestore;MatlabHome">
<Target Name="BuildDist" DependsOnTargets="BuildCppDist;NuGetRestore">
<MSBuild Projects="$(MSBuildThisFileDirectory)ice.sln"
BuildInParallel="true"
Properties="Platform=$(Platform);Configuration=$(Configuration);MatlabHome=$(MatlabHome)" />
Expand Down Expand Up @@ -105,7 +99,7 @@
</Task>
</UsingTask>

<Target Name="Package" DependsOnTargets="BuildDist;MatlabHome;MatlabVersion">
<Target Name="Package" DependsOnTargets="BuildDist">
<RemoveDir Directories="$(ToolboxDir)" />
<MakeDir Directories="$(ToolboxDir);$(ToolboxDir)\doc" />

Expand All @@ -124,14 +118,14 @@
<WriteFileWithReplacements InputFile="$(MSBuildThisFileDirectory)..\toolbox\info.template.xml"
OutputFile="$(MSBuildThisFileDirectory)..\toolbox\info.xml"
Tokens="@MatlabVersion@"
Replacements="R$(MatlabVersion)"/>
Replacements="$(MatlabVersion)"/>
<MSBuild Projects="ice.toolbox.targets"
Properties="Configuration=$(Configuration);Platform=x64" />
<Exec Command="&quot;$(MatlabHome)\bin\matlab.exe&quot; -nodesktop -nosplash -wait -log -minimize -r &quot;cd('$(MSBuildThisFileDirectory)..\toolbox');addFolderToPath '$(ToolboxDir)'&quot;"
<Exec Command="$(MatlabCommand) &quot;cd('$(MSBuildThisFileDirectory)..\toolbox');addFolderToPath '$(ToolboxDir)'&quot;"
WorkingDirectory="$(MSBuildThisFileDirectory)"/>
<Exec Command="&quot;$(MatlabHome)\bin\matlab.exe&quot; -nodesktop -nosplash -wait -log -minimize -r &quot;cd('$(MSBuildThisFileDirectory)..\toolbox');buildToolbox '3.8a0'&quot;"
<Exec Command="$(MatlabCommand) &quot;cd('$(MSBuildThisFileDirectory)..\toolbox');buildToolbox '3.8a0'&quot;"
WorkingDirectory="$(MSBuildThisFileDirectory)"/>
<Exec Command="&quot;$(MatlabHome)\bin\matlab.exe&quot; -nodesktop -nosplash -wait -log -minimize -r &quot;cd('$(MSBuildThisFileDirectory)..\toolbox');removeFolderFromPath '$(ToolboxDir)'&quot;"
<Exec Command="$(MatlabCommand) &quot;cd('$(MSBuildThisFileDirectory)..\toolbox');removeFolderFromPath '$(ToolboxDir)'&quot;"
WorkingDirectory="$(MSBuildThisFileDirectory)"/>
<Delete Files="toolbox.prj"/>
</Target>
Expand Down
2 changes: 1 addition & 1 deletion scripts/Util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4139,7 +4139,7 @@ def getCommandLine(self, current, process, exe, args):
" -wait -log -minimize" if isinstance(platform, Windows) else "",
)

return "{0} \"cd '{1}', runTest {2} {3} {4}\"".format(
return "{0} \"cd('{1}');runTest {2} {3} {4}\"".format(
matlabCmd,
os.path.abspath(
os.path.join(os.path.dirname(__file__), "..", "matlab", "test", "lib")
Expand Down
Loading