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.

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

Make OTP installation optional #79

Closed
michallepicki opened this issue Jan 4, 2022 · 9 comments · Fixed by #81
Closed

Make OTP installation optional #79

michallepicki opened this issue Jan 4, 2022 · 9 comments · Fixed by #81

Comments

@michallepicki
Copy link
Contributor

michallepicki commented Jan 4, 2022

Currently the otp-version argument is required. In some scenarios (e.g. when using custom OTP build from a different source, or compiling Javascript with Gleam, or only creating a Gleam project without running it etc), the action could still be useful - e.g. to install Elixir or Gleam.

Would it make sense to make the otp-version argument optional, similarly to other versions?

@starbelly
Copy link
Member

Anything is possible of course 😁 I suppose if we made it optional, then we would simply opt to grab a build against the latest stable version of OTP. Thus, if you said elixir 1.13.1 as an example, we would grab whatever the latest OTP 24 build for that version of elixir is. Is that the idea here?

@michallepicki
Copy link
Contributor Author

@starbelly If otp_version is not provided, the OTP would not be installed (assuming it already is installed, or is not needed). Does it make sense?

Alternatively it could be a otp_version: false setting

@paulo-ferraz-oliveira
Copy link
Collaborator

Alternatively it could be a otp_version: false setting

If we're to update anything, I'd probably go with this approach (a compulsory input). No default: we want it explicit that the action doesn't require OTP (which is the base for all the building process, as it stands).

@starbelly, thoughts?

@starbelly
Copy link
Member

I'd be ok with it, but here's the rub as FYI for others. We won't know what build of elixir to grab. A docker image, as an example may contain an incompatible version of OTP from what we grab for elixir. This might not be the case with gleam though?

If it's an option specific to gleam we might want to keep it that way, but for elixir it makes it all quite non-deterministic, such that people might use this option (or lack thereof), run into issues, open up issues here, etc.

Of course having a gleam specific option could be a tad confusing, I'm not against it, but it would need to be documented loudly.

@michallepicki
Copy link
Contributor Author

I didn't think of that. So it would be really a gleam-only feature, only for some use-cases of Gleam, and introducing a bit of complexity.

If you decide not to introduce this feature, Gleam still has an option to un-archive and keep maintaining gleam-lang/setup-gleam :)

@paulo-ferraz-oliveira
Copy link
Collaborator

If it's an option specific to gleam we might want to keep it that way, but for elixir it makes it all quite non-deterministic, such that people might use this option (or lack thereof), run into issues, open up issues here, etc.

This is what I'm afraid of, and also the cost of "more options" (though in this very specific case the number isn't increasing, just the change of error 😄). They come at a price! But this is also why I referred, if we're to implement this, we need to make sure the option is visible (eg. with a false as proposed by @michallepicki). As it stands, there's no version relation between OTP and Gleam, as there is for OTP and Elixir, but that's because that relation is kept outside the scope of this action.

So, at the most I'd say we could allow for otp-version: false iff gleam-version is defined, for now. Would that suit everybody? I mean, if we run into trouble there's always the possibility to rollback and/or search for alternatives.

@starbelly
Copy link
Member

I'm fine with it.

@paulo-ferraz-oliveira
Copy link
Collaborator

@michallepicki, we're accepting a pull request for this, then 😄, according to the previous discussion. Either that or wait for a while before I have time to tackle this.

@michallepicki
Copy link
Contributor Author

@paulo-ferraz-oliveira Thanks! I will try to hack it together and I'll report back in a few days.

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 a pull request may close this issue.

3 participants