-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Expand the capability of the :runtime option in Mix #6318
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
Conversation
|
Thanks @bitwalker. The concern is: should we do something in Mix if |
|
This does not provide suitable guarantees because a |
|
@josevalim Mix doesn't need to do anything with I'm not sure I follow @fishcakez's concern here, at least as far as what "guarantees" need to be provided here - my understanding is that when running under Mix, if some code executes which references a module which isn't in a loaded application, we attempt to load that module and then proceed. For example, if I open up a shell and run With releases it's a different situation:
The primary issue here is that there is no way for anyone to express that they want to load an application at runtime, but not automatically start it. The facilities for this already exist in OTP, in the release tooling - the problem is that there is nowhere in the Mix project file to express it. In my opinion, this PR is the cleanest fix which doesn't actually impact current behaviour with Mix, but provides release tools with enough information to correctly generate a release package with the desired properties. |
|
In mix, or anytime outside a release, It is also fragile because while the code for that dep might be available the modules are not guaranteed to work correctly after changing even patch version because it may start requiring supervision tree and that's not part of public API, it's an implementation detail. Also it is unclear what dependencies of a The way to handle this situation in mix is to use configuration and/or add functionality to start parts of tree later in the dep. This ensures all deps are available and working as intended. This may requiring patching deps. I understand that reltool (and relx) support this feature and it is convenient in short term to be available to a user but in my opinion this does not belong in mix and should be actively discouraged because the semantics are not good and behaviour would still be different in mix and releases. |
|
@tsloughter your expert opinion would be valued on load only applications, and why rebar3/relx behaves as it does. |
|
To be clear, The |
In |
|
@fishcakez Distillery and relx are separate tools, just FYI.
Seems to me, that if this is a gap we wish to close, we could do so by ensuring that Mix runs
I think you're missing the fact that developers are already doing this. Omitting applications from the applications list and dynamically loading them later (under Mix) or using a start type of
This is a separate issue - and apparently not one which is causing anyone problems at this point. I agree with what you are saying here though. The thing is that almost universally, developers are using "load-only" deps to lazily start some application (i.e. via |
I asked tristan's opinion because he has the most experience with build/release tools on BEAM. Always worth asking a knowledgable person :).
Unfortunately we can not ensure that mix loads these applications because it would not work consistently. For example in ExUnit with no start where a user manually starts applications. For this to work it would need to be built into
I think we should be discouraging people from doing this. Having a load only dep is fragile (for reasons previously stated) and ambiguous because it is uncertain what the intent is for deps of that dep. Do they need to be started? Loaded? Not even in path? One would assume loaded I guess.
It is not a separate issue because mix becomes forced to support it and we would want to prevent people getting into a situation where people are open to these bugs. Mix does not have the boot of a release so is open to many more issues. If that is the main use case then it sounds like we need to improve education on start phases. That already works in mix. |
Oh sure, no worries, I just thought I would mention it in case you thought Distillery was built on relx like exrm was.
I'm not sure I follow - when loading an application, you can simply load all of it's dependent applications as well, which is information that you can get via Using I think another point of confusion here is what application start types are used for in the first place - Erlang only uses them in releases, where they are used to guide the generation of instructions in the boot script (the
Well, I'm not so sure, maybe, except there isn't any alternative which allows you to lazily start applications (such as in cases where you need to do some configuration at runtime before you start an application). You can use start phases for that, along with As for what to do with deps of that dep, it's pretty clear to me that anything the application depends on which isn't already started or loaded by something else gets an effective start type of
There is already a problem! As it stands now, I'm left helping people figure out why things work in Mix but not in releases, because of Mix not guiding users in the right direction or in this case, essentially forcing them to abuse an option for something other than it's intended purpose to achieve something that was effectively not an issue in previous versions of Elixir:
If the opinion is "we would want to prevent people getting into a situation where people are open to these bugs", then I would like to point out that you are already failing in this regard, people are already open to this, and Mix is doing nothing to help. Of course, Mix can't do too much because applications with Just want to apologize if I'm coming across as rude - I really hope I'm not, I have the utmost respect for you all - I'm just a little confused why we're looking at this PR as if it opens us up to the issue that it's trying to address. At worst, all it does is reduce the cognitive dissonance when switching between Mix and releases - all of the problems you've raised are already an issue today, it's just that I get to shoulder the support burden ;) |
The problem is exactly this, the change does not work in mix but does work in releases. This PR is trying to move the problem from releases to applications but OTP does not support the feature at the applications level, only at the releases level. That means there is very little we can do at the mix level unless applications also support this. Therefore I don't think this approach is good for mix because we would be shipping a feature that doesn't work in mix so it can work in another tool. This is quite an uncompromising position I think. However, it is clear there is a problem to solve, I did not mean to imply there wasn't, and we do need to find some way to solve this. My opinion about the way to solve this, assuming it is delaying starting of applications, is to provide examples and material on start phases and included applications. Using a combination of these provides delayed startup, whilst also guaranteeing dependency ordering and works in mix and releases because it is at the application level. |
Yes, this is my point as well. |
|
With that justification I'm curious why we introduced the
I agree with this - I've got some documentation on this already, and am in the middle of writing a bunch of new material, including a bunch of examples of using these and how to solve a variety of use cases people have written me about. There are caveats though - in releases you cannot include an application which is already included by something else, but Mix will apparently let you do this no problem, but even if it didn't, you have to know to solve it by including the other application instead; you have to find out the application module for the application you are including and set up an unusual supervisor spec (i.e. If we want to require people to use
I'd be happy to tackle these items, but I want to make sure we're on the same page. |
|
When getting into the idea that mix raises an error regarding included applications or keeps Unless mix projects only ever have 1 release this may be fine, but that shouldn't necessarily be a restriction. So there may be project dependencies with an included application that aren't included in the release with another dependency that includes the application... if that makes sense. It can also be nice to have apps that you don't want in a release available for testing with the shell. |
|
I thought
I'm unsure how we can achieve this without breaking backwards compatibility, and many people might use a custom task, such as with phoenix. Also it would limit mix's dynamic nature, which is much more flexible than releases - we wouldn't want to lose that.
As @tsloughter mentions it is tricky for mix to warn about
If there are no start phases then it should never be called? I think it might be extremely rare for an application to include multiple applications and for a subset to require phases and others not. Even in that most complex of situations I guess one would have to use an intermediate app with
Given the most common behaviour suggested earlier it seems like only included applications would only usually be required and not phases, as people aren't doing that. I didn't check the docs before posting this but I'm sure there is room for improvement on both counts. That all being said using |
Right, it would be there to cover the case of when there are start phases.
I'm not sure that's true, I can think of an example pretty easily - you are building a release from an application in an umbrella where you include one of the other umbrella applications so that you can start a subset of the supervisor tree, but you also need to include one of your dependencies so that you can configure it at runtime - I have an application at work right now that does the former but not the latter, but all it would take to need the latter is if we happened to be using a dependency which needed configuration prior to starting, that's a super low bar to be considered "extremely rare" in my opinion.
Sure, that's an option, but I can't help but feel like it's a dirty hack which would not be at all obvious to most Elixir devs. I would hope that the language would provide options which make this stuff feel well integrated and less like you are doing something you aren't supposed to be doing.
I don't think I'd ever say patching some other library is "simple", especially for newcomers to the language just trying to get something into production. It's certainly doable, and you always have the option of maintaining your own fork, but it's horrible user experience. People will feel like they are doing something wrong because the "official" tooling doesn't prevent them from doing the wrong thing, but some random third-party (me) is telling them to do a bunch of other stuff in order to get their application working, when it "worked just fine when run with Mix" (I've heard that phrase sooo many times).
Examples of when using
No worries, this kind of discussion is important :) I can tell you definitively that while docs are great for telling users to RTFM, there are a very large number of people who don't even bother. Distillery's docs are as rich as ever, but it doesn't prevent people from almost daily opening new issues on the tracker trying to figure out why something they thought worked just fine is now broken when trying to run their release. Then I go read in blog posts or on the Elixir forum or Twitter that "deployment in Elixir is an obstacle to growth" because of the poor impression they have of how well integrated releases are with Elixir. Regardless of the fact that the tooling is well documented, very flexible, and is easy to get started with, these kind of pain points leave a huge negative impression that is very difficult to overcome. Sometimes making things harder on the tooling maintainers in order to improve the experience for users is a tradeoff worth making. We've certainly identified what the "right way" is in this discussion, but I want to emphasize that maybe we should consider the experience for users in the equation too. I can provide workarounds on the Distillery side so that people have an easy out, but it was my impression that supporting releases was an important goal for Mix - and because of that I generally avoid doing anything that widens the gap between how things are done in Mix and how they are done with releases - as it stands now, Mix doesn't really "support releases", it just doesn't not support them. The average release is more or less a 1:1 mapping with a Mix project - there are exceptions to that rule, including in my own projects, but I would say that it's easily the majority case. Those times where it's not the case, you have to specify the applications to include in the release manually anyway. Given that, I consider it the "happy path", where you should more or less be able to just run I'm going to close this PR for now - it seems like at this point it's not an acceptable solution to the team, which is fine - I'm just disappointed there doesn't seem to be a good way to close the gap. Unfortunately it means I'll have to paper over it in Distillery somehow. If you have suggestions on how you'd like to approach unifying the tooling, at least as far as making the experience feel tighter, I'm all ears and glad to help. Thanks for taking the time to talk it through! |
This is a release only configuration/feature, and it can't be handled at the application level because its not supported there in OTP. Mix can support something equivalent to: |
|
I wasn't aware of the Edit: Perhaps just start with |
|
It seems we have four states:
Unfortunately we cannot support option 3 right now because it wouldn't work in Mix nor across dependencies. I would say that Mix does not support 1, because if you have a dependency, it is always included, even with @bitwalker in your experience, which one is more likely to happen for users? 1 or 2? The only case one would expect Because I think that, regardless if we add more modes to Mix, we need to make |
|
I have some code generation libraries that do not nor should not really exist at runtime. Option 1 makes sense for me. In fact if there was an option to only load a library in at 'compile-time' (macro's and so forth) but not exist at mix runtime I would be quite happy, especially if mix itself could be in that list somehow (I know it can't, just bikeshedding there) so people quit accidentally using it then trying to Release while making Mix calls. |
From #5473: "Actually, :start is not good because you can have an application that you want included in a release but not started. However, this option is doing both: it is not including it in the release and not starting it.". That's @josevalim explaining why the choice to use
There are two different reasons users use
While that's an option, the first thing I'm going to hear from users is "I set
My plan at this point is to let users override |
- Previously, the purpose of `runtime: true | false` was to specify whether a given application should be included in the runtime applications list. However, this presents problems when running projects under Mix versus under releases, as setting `runtime: false` would effectively allow you to treat that application as if it had a start type of `:load` - however in releases, `runtime: false` would cause that application to be excluded from the release, putting the user in the position of needing to manually curate the applications list in order to have a "load-only" application work correctly in both setups. - This change adds two additional `:runtime` option values, `:none` and `:start`. `false` behaves like it did before, and while `true` is supported for backwards compatibility here, `:start` takes it's place as the new default value. We also introduce `:none` which is effectively the same as `false` when running a project under Mix, but allows release tools (such as Distillery) to specify the start type of the application to use when compiling the release.
569d14e to
ef6122f
Compare
|
Thanks @bitwalker! Something I also noticed that is that the confusion between 1 and 2 also happens with OTP and Elixir, where you can load at them any time inside Mix, even with not listing them, but not inside releases. Given this symptom is wide-spread and, per your feedback, 2 and 3 would be the same, we are back to the |
Mix does not have "load-only" semantics and can not support it without OTP support. We have been using the term load where it strictly means 2 and 3 are very similar but definitely not the same. I think it would be a mistake to change a Also I think it would only make sense to make this change ( |
To be clear, I know that, it's why I'm air-quoting it :P. People are setting
I don't think it would confuse people nearly as much as the current situation.
Distillery can certainly do that, except releases run in embedded mode, so It's unfortunate we'll have to continue with this awkward difference between Mix and releases, but I'll see about putting in a PR to OTP for something along the lines of |
The confusion is worse because bugs can be hidden and non obvious as it code may work but have wrong configuration, whereas the current situation would lead to a crash.
Are you sure this is true? |
|
I just tested a few releases and can't find how |
Per my conversation with @josevalim earlier today in IRC, this fix is intended to address a discrepancy between how projects run under Mix versus releases.
Previously, the purpose of
runtime: true | falsewas to specify whether a given application should be included in the runtime applications list. However, this presents problems when running projects under Mix versus under releases. When run via Mix, settingruntime: falsewould result in the application not being started (because it's not in the runtime applications list), but would allow you to still load/start it at runtime (because Mix allows you to dynamically load applications which are on the code path). When in a release, the application is not even available to load as it was excluded from the release due to settingruntime: false. In other words,true | falseis not granular enough to express the intermediate state, where you want the dependency included at runtime, but not started.This change adds two additional
:runtimeoption values,:loadand:start.falsebehaves like it did before, andtrueis supported for backwards compatibility here (it has the same meaning as:start),:starttakes it's place as the new default value. We also introduce:loadwhich is effectively the same asfalsewhen running a project under Mix, but allows release tools (such as Distillery) to specify the start type of the application to use when compiling the release.