Skip to content

Add a source argument to setup-matlab #149

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

sameagen-MW
Copy link
Contributor

This source argument acts as a pass through to mpm's --source option. There are a couple of caveats for using it:

  1. If both release and source are specified, source is used for the installation and release is ignored.
  2. source is not supported with caching. If the cache argument is used with the source argument it will be ignored.
  3. Installations using the source argument will not install system dependencies, even on GitHub hosted runners.

Most of these caveats stem from the fact that we're unable to get the MATLAB version from the path provided by the source argument. This means we're unable to determine the correct dependencies to install. This is an area where there could be some discovery work in the future.

@sameagen-MW
Copy link
Contributor Author

Qualified on Windows and Linux: https://github.com/sameagen-MW/v1-doctoring/actions

Seems like there's currently a bit of an outage with GHA, haven't been able to get a Unix runner scheduled in the last 45 mins.

@@ -9,6 +9,11 @@ inputs:
MATLAB release to set up (R2021a or later)
required: false
default: latest
source:
Copy link
Member

Choose a reason for hiding this comment

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

@sameagen-MW: Is this going to be an internal feature or are we going to document the input right away?

(https://www.mathworks.com/help/install/ug/mpminstall.html#mw_5abeb5ba-92a4-4ecf-8e4d-263760988c7e)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the goal is to document it right away. It has some niche use cases with self-hosted runners, but also acts as a mitigating factor if there's ever any future outages.

src/install.ts Outdated
Comment on lines 80 to 88
const releaseKey = 'source-' + crypto.createHash('sha256').update(source).digest('hex');
const releaseInfo = {
name: name,
version: releaseKey,
update: "",
isPrerelease: false
};

const [destination, alreadyExists]: [string, boolean] = await matlab.getToolcacheDir(platform, releaseInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with the toolcache? I think last time I interacted with it, it required the release to follow semantic versioning when looking up an entry or it returned null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah great catch. They should add that as a precondition on tc.find. You're totally right, they call semver.clean here which will end up setting the version spec to null and always returning null for the toolpath.

Comment on lines +80 to +85
const releaseInfo = {
name: releaseKey,
version: "1.0.0",
update: "",
isPrerelease: false
};
Copy link
Member

Choose a reason for hiding this comment

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

What about looking for this info in ProductFilesInfo.xml (should exist when using an mpm download folder) or VersionInfo.xml (should exist when using a mounted ISO folder)? Then we could install system dependencies and place MATLAB in the right tool cache location. If we can't find either file (which would probably indicate a corrupt or incorrect source folder anyway), this approach could be a fallback. We do something similar in run-matlab-command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants