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

support for optional_applications #2473

Closed
tsloughter opened this issue Jan 19, 2021 · 29 comments
Closed

support for optional_applications #2473

tsloughter opened this issue Jan 19, 2021 · 29 comments
Labels
enhancement new behaviour or additional functionality

Comments

@tsloughter
Copy link
Collaborator

Support for optional_applications will be merged into OTP soon: erlang/otp#2675

I have opened an issue for supporting this in the hex plugin, erlef/rebar3_hex#195

I'm not actually sure if we need to do anything in rebar3 to support this because of how we resolve dependencies -- unlike mix where the optional deps version constraint is taken into account when deciding what version to pull of a dep.

I suppose one issue is if the user doesn't want to list all the optional deps in the project's default profile. If that were the case we'd be basing the optional deps published to hex solely on the optional_applications list which wouldn't have a version constraint (not that it matters I guess) or place to put the hex package name if it differs from the application name...

Another case is if the project is pulled in as a git dependency. Does it then simply fetch all the deps, even those that are technically optional at runtime?

@tsloughter tsloughter added the enhancement new behaviour or additional functionality label Jan 19, 2021
@ferd
Copy link
Collaborator

ferd commented Jan 19, 2021

Optional dependencies is a run-time constraint, not a compile time one, so the thing here is that we should still fetch and build them, but we might just not yell if they're not found when it comes to assembling releases or starting a project, as far as I can tell.

@tsloughter
Copy link
Collaborator Author

The app file additional is a runtime constraint, doesn't mean we can't provide compile time features around it. It probably isn't worth it so I'm fine leaving it as is. Though probably worth it add support for non-hex deps to not fetch optional deps, but leaving it so when working on the application itself all deps are fetched.

@tsloughter
Copy link
Collaborator Author

Added in latest release.

@ThomasArts
Copy link

In my opinion there is some release handling needed for optional_applications. Not sure whether release building is rebar3 or relx thingy, but somehow one needs to be able to say when building a release whether or not to include an optional application in it. (At the moment it is always added, but in a discussion of "wx" we really don't want all optional applications always shipped in a release).

https://erlangforums.com/t/erlang-otp-27-0-rc1-released/3300

@uwiger
Copy link
Contributor

uwiger commented Feb 21, 2024

I think it could be useful to be able to specify three different types of 'release' specification:

  • fully qualified: only consider the apps actually mentioned, and if a mandatory dep is missing, fail
  • semi-qualified: automatically wink in required dependencies, but no optional ones, unless they are listed in the spec
  • auto: the way it works today

The way the code is structured, rebar_relx.erl seems like the best place to do this.

@MarkoMin
Copy link
Contributor

Isn't that handled with {exclude_apps, [app1, app2]} in the relx entry? I wouldn't say that one wants minimal release by default - I'd say that one wants working release by default. Eventually, we could add something like {include_optional_applications, boolean()} to have an option to have a minimal release (not guaranteed to be working) and a working release (with a possibility of bloat). That combined with exclude_apps gives all the control one might need.

P.S. The problem is that it's not really clear what optional dependencies actually are. They can be not so optional from time to time - when a missing application causes root application to error/exit is it optional or not?

@ferd
Copy link
Collaborator

ferd commented Feb 21, 2024

So far they've been considered optional in that we're always attempting to include them, but it's not an error if they can't be found or if the release specifies they should not be brought in.

One could easily not make it an error by simply excluding them from the relx config, as @MarkoMin points out.

You could, for example, have a default release like:

{relx, [{release, {myrel, "0.1.0"},
         [myapp, {aws, load}, sasl]},
        {exclude_apps, [wx]}
]}.

and then make an extra profile like:

{profiles, [
    {wx, [
        {relx, [{release, {myrel_wx, "0.1.0"},
                 % the 'wx' below is optional, if `myapp' depends on it, it would get included anyway
                 [myapp, {aws, load}, sasl, wx]}
        ]}.
    ]}
]}

and every build by default (rebar3 release) will ignore wx, and every build with the explicit profile (rebar3 as wx release) should include it. (I haven't actually tested the configs above, as I'm writing this right after coffee/before breakfast). If I recall correctly, there can be edge cases where if the app is marked optional by a dep but is also marked as necessary at the top level or by a non-optional inclusion, then missing the app will be considered a failure.

@uwiger
Copy link
Contributor

uwiger commented Feb 21, 2024

I wouldn't say that one wants minimal release by default - I'd say that one wants working release by default.

Well, compare to embedded code loading: What one typically wants, apart from (in that case) avoiding incurring the cost of on-demand code loading, is the assurance that the system doesn't work accidentally due to specifics of the environment, only to later fail on another, where some local modules aren't picked up due to local paths and on-demand code loading.

In this particular case, that's exactly what happened when testing the Aeternity code base on OTP 27. It failed because on the machine being tested, wx didn't load cleanly. Had wx happened to be properly installed on Hans Svensson's machine, we might have missed the fact that wx was now being winked in, and failures would have manifested later, as wx is notoriously problematic, esp. in embedded systems that are unlikely to have the requisite packages installed.

Obviously, there also wasn't any {exclude_apps, [wx]}. The project had a dependency to observer, since trace_runner makes use of ttb.

In such cases, it would be much better to have the build fail early. But I could have also clarified that the default behavior could remain as today, just as on-demand code loading is the default when starting Erlang.

@ferd
Copy link
Collaborator

ferd commented Feb 21, 2024

Yeah I could imagine {include_optional_applications, boolean()} as @MarkoMin suggested doing that work of setting more comfortable defaults for these minimal builds that don't incidentally include more apps than they used to.

(It wouldn't cover the case of third party libs suddenly incorporating more entries, which still get to be part of supply-chain checks for corporate entities, but would help for truly optional things).

The gotcha here though is that this would be a setting easiest to add for releases; doing it for tests, dialyzer, xref, and dependency fetching would be significantly more complex, and I'm not quite sure at which level "not fetching in optional deps" would be most "correct". I can easily imagine it being the case for tests (when calling to boot dependent apps) also being a pain, but being much trickier to support well (based on messing around with the lock file when switching from desired to undesired for example, or also calls within OTP like application:ensure_all_started/1 that do not take this in consideration).

@uwiger
Copy link
Contributor

uwiger commented Feb 21, 2024

Yeah I could imagine {include_optional_applications, boolean()} as @MarkoMin suggested doing that work of setting more comfortable defaults for these minimal builds that don't incidentally include more apps than they used to.

A way to selectively include some optional applications under that setting would then be to specifically include them in the ´release´ apps list.

@MarkoMin
Copy link
Contributor

What one typically wants, apart from (in that case) avoiding incurring the cost of on-demand code loading, is the assurance that the system doesn't work accidentally due to specifics of the environment

I definitely agree, but it seems that your system was accidentally working before OTP27. wx is a runtime dependency of observer, meaning that it needs wx to fully operate. When I include observer in the release, I expect it to work as usual. However, if I notice that I don't use part of observer which requires wx, then I can manually exclude it.

Had wx happened to be properly installed on Hans Svensson's machine, we might have missed the fact that wx was now being winked in, and failures would have manifested later, as wx is notoriously problematic, esp. in embedded systems that are unlikely to have the requisite packages installed.

Under what code path? Rebar sets the code path to <rel_root>/lib/*, how can wx happen to be there?

Yeah I could imagine {include_optional_applications, boolean()} as @MarkoMin suggested doing that work of setting more comfortable defaults for these minimal builds that don't incidentally include more apps than they used to.

It would fit nicely with {mode, minimal} :D

The gotcha here though is that this would be a setting easiest to add for releases; doing it for tests, dialyzer, xref, and dependency fetching would be significantly more complex, and I'm not quite sure at which level "not fetching in optional deps" would be most "correct"

If you want to test your app without some apps, can't you do it in a profile that redefines deps entry?

A way to selectively include some optional applications under that setting would then be to specifically include them in the ´release´ apps list.

exactly

@ferd
Copy link
Collaborator

ferd commented Feb 21, 2024

If you want to test your app without some apps, can't you do it in a profile that redefines deps entry?

Not if a transitive dep of a critical one includes the optional one in its own deps list or lock file, because then the app is already not in your config. You'd need something weirder like use override features to redefine that dep's deps list to not include any of these optional ones.

@uwiger
Copy link
Contributor

uwiger commented Feb 21, 2024

I definitely agree, but it seems that your system was accidentally working before OTP27. wx is a runtime dependency of observer, meaning that it needs wx to fully operate.

For observer to fully operate, yes, but we were only using ttb, which is an extension of dbg and happens to reside in observer - it probably shouldn't. The same could be said for etop, which is an excellent utility for embedded systems, and has nothing to do with wx.

@MarkoMin
Copy link
Contributor

For observer to fully operate, yes, but we were only using ttb, which is an extension of dbg and happens to reside in observer - it probably shouldn't. The same could be said for etop, which is an excellent utility for embedded systems, and has nothing to do with wx.

They could be separate applications I guess, but that's a separate problem. Try to reach OTP team, but I don't know if they'll fit it in OTP27

@MarkoMin
Copy link
Contributor

MarkoMin commented Feb 22, 2024

The gotcha here though is that this would be a setting easiest to add for releases; doing it for tests, dialyzer, xref, and dependency fetching would be significantly more complex, and I'm not quite sure at which level "not fetching in optional deps" would be most "correct".

I just peeked over source and indeed, that would be a lot of work to support include_apps, exclude_apps and include_optional_apps for every provider separately. One (harder, yet more valuable) way to achieve this would be inject those 3 somewhere deep (e.g. rebar_prv_app_discovery) and then all other providers would just rely on that. Benefit would be: 1. one config entry all providers, 2. all providers would have feature to incl/excl apps (one could also support incl/excl of modules). Currently some providers have option to incl/excl opts, but not all.

Example of how this might work:

%%rebar.config
{profile, 
   [{minimal, [{app_discovery, [{include_optional_applications, false}, {include_apps, [wx]}]
                      ]
]}.

and then you could run rebar3 as minimal ct/dialyzer/....

@ferd
Copy link
Collaborator

ferd commented Feb 22, 2024

Yeah, although part of the challenge there is that knowing what is optional or not requires evaluating the .app file, and this file can be changed up to final compilation steps under the current model, so even unwinding that can be quite tricky (see #2864 for example, opened this week, about hooks running after compilation modifying the app files and wanting to support that when building releases)

I don't know for sure that we could support this without making it a breaking change that reevaluates how we interleave build steps far more aggressively. We're sort of caught by having picked a model that didn't care for this feature at first (because it didn't exist).

@ferd
Copy link
Collaborator

ferd commented Feb 22, 2024

A less brutal change could be to be to hook ourselves into rebar_paths and never load optional modules according to some project-level option. Currently it knows of deps, plugins, and runtime options (runtime is like deps but restricts choices to only what is declared in the app files. I think only relx uses them right now). A non_optional option would likely be like runtime minus the optional apps.

I'm not exactly sure how it could be invoked, but it could look and test things properly for code analysis that depends on the path. Unfortunately, this doesn't involve relx, which builds releases without needing to load the apps and has its own resolution mechanism.

Also on another note: the rebar_paths stuff is an incredible pain in the ass to test and I'm nervous about touching this code because it took us months of refactoring to get it to a workable state, so I'm not 100% sure it is actually "less brutal".

@MarkoMin
Copy link
Contributor

A less brutal change could be to be to hook ourselves into rebar_paths and never load optional modules according to some project-level option. Currently it knows of deps, plugins, and runtime options (runtime is like deps but restricts choices to only what is declared in the app files. I think only relx uses them right now). A non_optional option would likely be like runtime minus the optional apps.

The same reasoning can then be used for relx, one could easily modify .app files after the release is packaged... I think .app.src is where everything relevant to .app files should happen. If one modifies the .app file mid-compilation it could break literally anything. I can see the need for injecting any .app property from file or some other resource and the need for calculating version after compile, but modifying dependencies after compilation doesn't seem like a valid case to me (in some edge cases maybe you want to modify them pre-compilation, but I can't see it done after the compilation or any other stage after that).

@ferd
Copy link
Collaborator

ferd commented Feb 22, 2024

What I think is or isn't valid cases turns out to not be really relevant to how people use this tool :/ Three quarters of the work is figuring out how to make shit work for use cases you never thought would make much sense but that someone has a really good edge case for sometimes.

@josevalim
Copy link
Contributor

So far they've been considered optional in that we're always attempting to include them, but it's not an error if they can't be found or if the release specifies they should not be brought in.

My understanding is that an optional application must not be started (and I would say not even included in the release) unless someone else depends on it as a non-optional application.

Here is what the docs say:

optional_applications
A list of applications that are optional. Note if you want an optional dependency to be automatically started before the current application whenever it is available, it must be listed on both applications and optional_applications.

if you automatically bring optional applications into a release, and therefore start them, one could argue they won’t match the behavior above? Optional applications are to enforce order, but not a requirement.

@MarkoMin
Copy link
Contributor

if you automatically bring optional applications into a release, and therefore start them, one could argue they won’t match the behavior above? Optional applications are to enforce order, but not a requirement.

Including them in a release != starting them (although if I remember correctly there was a bug case where optional apps got started even though they weren't listed in any applications list). Optional and included applications must be there, but are not started automatically.

@josevalim
Copy link
Contributor

Thank you. Not starting them definitely helps. So one last questions: what about Hex packages? If I have a dependency as optional on Hex, do you download it? Even if somebody does not depend on it?

If the answer is no, it means optional hex packages are not even included in releases. Why would optional deps from OTP be included?

If the answer is yes, should you download packages that are optional? Then what is the difference then between optional and included?

@MarkoMin
Copy link
Contributor

MarkoMin commented Feb 28, 2024

Thank you. Not starting them definitely helps. So one last questions: what about Hex packages? If I have a dependency as optional on Hex, do you download it? Even if somebody does not depend on it?

If you have that dependency in your deps in rebar.config and if dep fetch succeeds - then yes. If you don't have it listed there - then no.

Naming is really unfortunate here and optional_applicatinons key is abused a lot. included_applications doesn't mean "load an application, but don't start it", it means "load an application and I will start it and no other application can have any dependency on the included application". Therefore, two applications can't "include" the same app. optional_applications is used to "load an application, but don't start it" (while ensuring that those applications are available).

I dont like that mechanism, but it currently works. I remember being confused by it when I started doing Erlang and I still am. What we're lacking is "include an application, but don't start it" mechanism (we're currently using optional_applications for that).

If the answer is yes, should you download packages that are optional? Then what is the difference then between optional and included?

Yes, because they are not exactly optional (cuz of the abuse).

@tsloughter
Copy link
Collaborator Author

We could require optional applications be listed in the list of release applications in order to be included in the release.

So, from the first example in this thread, instead of using exclude_apps it would be, no wx:

{relx, [{release, {myrel, "0.1.0"},
         [myapp, {aws, load}, sasl]}
]}.

Yes wx:

{relx, [{release, {myrel, "0.1.0"},
         [myapp, {aws, load}, sasl], wx}
]}.

@MarkoMin
Copy link
Contributor

We could require optional applications be listed in the list of release applications in order to be included in the release.

In theory yes, in practice it's a tradeoff (again, because of the abuse above) - do you want sane releases (system_information:sanity_check/0) or minimal releases.

@josevalim
Copy link
Contributor

@MarkoMin curiosity: why would @tsloughter's suggestion fail the sanity check?

@MarkoMin
Copy link
Contributor

Example:

  1. have a release with [stdlib, kernel, observer] only.
  2. make a release
  3. deploy a release
  4. try to run observer:start() on a release
  5. get a bunch of errors

Why 5.? Because wx is optional dependency of observer and therefore isn't included in the release, but is a runtime dependency. Isn't that contradictory - same app is both optional and runtime-dependency? I argued (somewhere) before that runtime-depenedenices should be included in the release by default, by contra-argument was that runtime-dependenices is used mostly only from within OTP (but optional_applications aren't).

@josevalim
Copy link
Contributor

josevalim commented Feb 28, 2024

I'd say that's par for the course for optional dependencies. If I have an optional dependency, certain functionality won't be available, and it will raise at runtime unless the dependency is available. I believe Rust also behaves similarly with its optional dependencies via features, where whole modules or functions become undefined.

I guess the big question is if you expect it to fail or not? You are arguing that someone could be surprised that observer does not work unless they do additional work by listing additional deps?

@MarkoMin
Copy link
Contributor

Yes, but when runtime error occurs it can be significantly hard to tell what part of the system is missing! Ideally, you'll get {undef, ... error, but you can also get stuck with more cryptic errors that are caused because optional dep of optional dep of optional dep is missing. Some of those errors may manifest after a while - so what then? You thought your release is working, but now you gotta deploy again. And I can see that "oh I forgot app X" loop in the release lifecycle.

Removing optional apps from the release is more or less just an optimization - it's up to the user to conclude whether it is safe to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new behaviour or additional functionality
Projects
None yet
Development

No branches or pull requests

6 participants