search

mamba-org / setup-micromamba

GitHub Action to set up micromamba
MIT License
97 stars 15 forks source link

Update micromamba installation process, refactor options #185

Closed 0xbe7a closed 8 months ago

0xbe7a commented 9 months ago

This pull request updates the micromamba installation process to use a pre-installed binary if available and micromamba-binary-path and download-micromamba are undefined or false.

pavelzw commented 9 months ago

Do we need new test cases?

Also, can you bump the version to 1.8.0 in package.json?

0xbe7a commented 9 months ago
Micromamba on $PATH download-micromamba micromamba-binary-path Behavior
No unset unset Download to default location
No unset set Download to specified path
No false unset Error
No false set Use binary (absolute path)
No true unset Download to default location
No true set Download to specified path
Yes unset unset Use binary (from PATH)
Yes unset set Download to specified path
Yes false unset Use binary (from PATH)
Yes false set Use binary (specified path)
Yes true unset Download to default location
Yes true set Download to specified path
pavelzw commented 9 months ago

micromamba-binary-path always causes download = false and fails if no binary file exists at this location.

this would be a breaking change. Also I don't know if we really want to have this behavior. In some other self-hosted scenarios (non-ephermal), you might want to explicitly alter the path to ${{ runner.temp }} and download the binary there (see readme)

pavelzw commented 9 months ago

I like the table, how about we also put it in the readme?

pavelzw commented 9 months ago

The table doesn't cover what's happening when download-micromamba is unspecified.

0xbe7a commented 9 months ago

I extended the table above and in the README

jonashaag commented 9 months ago

If you need a table to explain the interaction between the options, maybe there is too much interaction going on. Is there a way to simplify things?

0xbe7a commented 9 months ago

If you need a table to explain the interaction between the options, maybe there is too much interaction going on. Is there a way to simplify things?

I would want the following use-cases / options for users:

I can't think of any real simpler solutions that allow all use cases without introducing breaking changes. I also think every row in the table matches the expected behaviour.

Do you have ideas how we could simplify it?

jonashaag commented 9 months ago

IIUC this is the only new feature of this PR:

Use Micromamba from PATH if it exists and no special option is set

And the matrix without this PR is

Micromamba on $PATH download-micromamba micromamba-binary-path Behavior
X false unset Use binary (from PATH, error if missing)
X false set Use binary (absolute path)
X true/unset unset Download to default location
X true/unset set Download to specified path

Simplified, the old cases are

With the new feature, the matrix gains 6 new cases.

Simplified, the new cases are

Not easy to reason about. I think it's a very high cost just for being able to remove these lines from the caller:

with:
  micromamba-binary-path: micromamba
  download-micromamba: false

Or are trying to solve situations here where we don't know whether Micromamba is in PATH, ie. we can't hardcode these two lines?

0xbe7a commented 9 months ago

Or are trying to solve situations here where we don't know whether Micromamba is in PATH, ie. we can't hardcode these two lines?

No, my intention for this PR is to be able to remove those lines.

I am using GitHub actions with self-hosted runners in an environment with limited network access. I use several composite actions that use setup-micromamba themselves. Without this PR, each of these actions would have to re-export an "offline" parameter in order to be used in this environment. With this PR, everything would work out-of-the-box, both for runners with pre-installed Micromamba and for runners without Micromamba.

Also, I don't think "Micromamba on path and binary path unset" is that costly for this feature compared to the alternative of having to customize most of the downstream actions and probably the behavior most users would want, after they installed Micromamba into their runner.

0xbe7a commented 8 months ago

I have added a test to ensure that if download-micromamba: true the pre-installed version is ignored and the pre-installed micromamba is used by default.

During this i noticed that setup-micromamba tries to create the micromamba-shell right next to the micromamba binary.

This should probably be changed to a tmp path, as micromamba might be installed into e.g. /usr/local/bin