search

OvermindDL1 / protocol_ex

Elixir Extended Protocol
https://hex.pm/packages/protocol_ex
48 stars 5 forks source link

Some (many) protocol ex implementations do not get accounted for on Elixir 1.15+ #67

Open DaTrader opened 4 months ago

DaTrader commented 4 months ago

This regards the problem I first mentioned here yesterday: https://elixirforum.com/t/protocolex-extended-protocol-library-using-matchers/6822/10

I use ProtocolEx extensively, so it's all over my (umbrella) project, both in the domain and the web apps. The issue is that the tests in your repo pass while in the real world app, ProtocolEx is no longer accounting for some of the defimpl_ex implementations. And it's doing so virtually randomly.

At first, it didn't account for most (maybe even all) defimpl_ex in the web app of the umbrella project b/c the dependency and the compiler was only set in the domain app (that still used to work 100% ok on Elixir 1.14.5). So, I added those to the web app mix.exs as well. However, it only partially fixed the problem for some of the defimpl_ex in the web app are still not accounted for although despite the fact they all get compiled.

It's obvious this is due to some changes introduced in Elixir 1.15, but it's extremely hard for me to understand which exactly.

DaTrader commented 4 months ago

UPDATE:

  1. I realized the pattern of ProtocolEx ignoring the defimpl_ex (now that both apps have the protocol_ex in the compiler line and as a dependency):

    • If my web app in the umbrella project defines a defimpl_ex of a ProtocolEx module in my domain app, the implementation is not going to be accounted for i.e. it's not gonna be there in the list returned by the AProtocolExModule.__proto_ex_impls__/0

  2. Another issue is that each time I move a defimpl_ex from one app in the umbrella project to another (as in the case above) I need to delete the _build folder before recompiling or otherwise the web app defimpl_ex implementations will be empty for all ProtocolEx's used there. The compile --force option no longer helps as it used to in similar cases prior to Elixir 1.15

DaTrader commented 4 months ago

UPDATE 2:

Actually, the second issue is much worse than suggested above. The _build folder must be deleted before compiling with --force every single time, or otherwise the defimpl_ex implementations don't get accounted for.

DaTrader commented 4 months ago

@OvermindDL1 I reported this same issue in the Elixir repo and I have just received the response from Jose' where he pinpoints the problem with the library.

Please check the following link: https://github.com/elixir-lang/elixir/issues/13588#issuecomment-2127861916

OvermindDL1 commented 4 months ago

Oh fascinating! Thanks for the ping, I actually get those fast (notification overload spam on github sucks...). :-)

Let me look in to this...

When you are inside an umbrella project, and you run mix test inside demo, you have only a subset of all dependencies. In versions prior to v1.15, we did not prune code paths, which meant you may or may not have demo_web in your code path. Elixir v1.15 makes it consistent, demo_web is never available when compiling demo, so the bug is always reproducible, since protocol_ex compiles protocols inside demo first (the first dependency to compile).

Hrmmm....

The way we solved this for Elixir protocols is to have three distinct consolidation paths, one per app, and one for the umbrella. The library would have to mirror that, instead of relying on a single application having a global view of the system (which was not true before and it is deterministically not true now).

Yeah I never used this in an umbrella, rather just a normal singular project with normal deps (I've never been a fan of monorepos like umbrellas encourage so never tried it)...

Let me catch up on some things in this repo first...

OvermindDL1 commented 4 months ago

Hmm, this appears to be more troublesome than originally anticipated. Since this isn't the elixir compiler itself it doesn't have a view over the entire umbrella, which may not even be entirely compiled, and rather the compiler itself is only giving a subset of the views. I'm not sure how else to get the entire view if the compiler itself is only giving ProtocolEx a subset of the views... The crux of it seems to come down to this little function in ProtocolEx:

  defp get_base_paths(opts) do
    case opts[:ebin_root] do
      nil -> :code.get_path()
      paths -> List.wrap(paths)
    end
  end

I query if an ebin_root is passed, if so it uses it, else it uses :code.get_path(), which should return all loaded code paths, but the issue is that elixir isn't loading all the code paths for an umbrella project (which makes sense when you only need a subset) but makes it difficult to get the non-loaded paths since they, well, aren't actually depended on. So the current functionality 'seems' correct? In essence if a ProtocolEx implementation needs to be implemented in the current build then the library it's in needs to be depended on, and I'm unsure how else that would even work...

I feel like I'm misunderstanding how your environment is set up actually. For my little test here, (hmm, actually let me commit it right quick, done, it's now in test_extras) I have 3 libraries and one application in this umbrella, labelled lib1, lib2, lib3, and app1.

:lib1 contains the protocol definition (and some basic implementations) :lib2 contains basically nothing (I was confirming with some pathing) :lib3 contains another implementation for an in-built type (floats) for lib1's implementation, lib3 depends on lib1.

:app depends on lib1 and lib2, but not lib3 (to try to mirror what I gathered from your above comments and the other linked github thread), and it has tests for the existing functionality as well as for the float implementation, which as expected fails (I would not expect it to work since app1 does not depends on lib3 transiently, recursively, or directly). I run mix test from within test_extras/umbrella/apps/app1 and it fails as expected on the float test (ran the fallback implementation)

But this is how I would expect this to work, an implementation that is not depended on anywhere in the dependency graph does not exist 'to' work (as if it, for example, called a function within itself then it wouldn't be loaded in to even called).

Is this not how yours is set up? Can you give me a very minimal example that replicates your issue if not?

OvermindDL1 commented 4 months ago

I ran:

git clone https://github.com/DaTrader/demo_umbrella_1_15.git && cd demo_umbrella_1_15
mix deps.get
mix test

And got the failing testcase.

Changed the protocol_ex dep to point to me local version, wiped all _builds, mix test again, and I see it processing these paths as loaded in to the system:

==> demo
Compiling 4 files (.ex)
Generated demo app
Paths: [~c"/home/overminddl1/elixir/tmp/demo_umbrella_1_15/_build/test/lib/demo/ebin",
 ~c"/home/overminddl1/.asdf/installs/erlang/27.0/lib/stdlib-6.0/ebin",
 ~c"/home/overminddl1/.asdf/installs/erlang/27.0/lib/kernel-10.0/ebin",
 ~c"/home/overminddl1/.asdf/installs/erlang/27.0/lib/crypto-5.5/ebin",
 ~c"/home/overminddl1/.asdf/installs/erlang/27.0/lib/runtime_tools-2.1/ebin",
 ~c"/home/overminddl1/elixir/tmp/demo_umbrella_1_15/_build/test/lib/dns_cluster/ebin",
 ~c"/home/overminddl1/elixir/tmp/demo_umbrella_1_15/_build/test/lib/protocol_ex/ebin",
 ~c"/home/overminddl1/elixir/tmp/demo_umbrella_1_15/_build/test/lib/phoenix_pubsub/ebin",
 ~c"/home/overminddl1/.asdf/installs/elixir/1.16.3-otp-26/bin/../lib/elixir/ebin",
 ~c"/home/overminddl1/.asdf/installs/elixir/1.16.3-otp-26/bin/../lib/mix/ebin",
 ~c"/home/overminddl1/.asdf/installs/elixir/1.16.3-otp-26/bin/../lib/logger/ebin",
 ~c"/home/overminddl1/.asdf/installs/elixir/1.16.3-otp-26/bin/../lib/iex/ebin",
 ~c"/home/overminddl1/.asdf/installs/elixir/1.16.3-otp-26/bin/../lib/ex_unit/ebin",
 ~c"/home/overminddl1/.asdf/installs/elixir/1.16.3-otp-26/bin/../lib/eex/ebin",
 ~c".", ~c"/home/overminddl1/.asdf/installs/erlang/27.0/lib/erts-15.0/ebin",
 ~c"/home/overminddl1/.asdf/installs/erlang/27.0/lib/compiler-8.5/ebin"]
==> demo_web
Compiling 13 files (.ex)
Generated demo_web app

So it generated the implementation within 'demo', though it doesn't seem like it was called from within demo_web, even though demo_web has the compiler registered as well, let's see....

OvermindDL1 commented 4 months ago

Looks like because 'demo' is comprehensive and 'demo_web' isn't doing much, hmm...

If I remove the protocol_ex compile from 'demo' so it's just in demo_web since demo_web contains demo then it does indeed compile there:

==> demo_web
Compiling 12 files (.ex)
Compiled lib/demo_web/application.ex
Compiled lib/demo_web/controllers/error_json.ex
Compiled lib/demo_web.ex
Compiled lib/demo_web/telemetry.ex
Compiled lib/demo_web/bar.ex
Compiled lib/demo_web/controllers/page_controller.ex
Compiled lib/demo_web/endpoint.ex
Compiled lib/demo_web/router.ex
Compiled lib/demo_web/components/core_components.ex
Compiled lib/demo_web/controllers/error_html.ex
Compiled lib/demo_web/controllers/page_html.ex
Compiled lib/demo_web/components/layouts.ex
Generated demo_web app
Consolidating ProtocolEx's project-wide...
ProtocolEx beam module Elixir.Demo.Fooable.beam with implementations [Demo.Fooable.DemoWebBar, Demo.Fooable.DemoFoo]
Consolidating ProtocolEx's project-wide complete.
Consolidated Plug.Exception

But if the compiler is in both then it gets invoked on demo, but not demo_web:

==> demo
Compiling 4 files (.ex)
Compiled lib/demo/application.ex
Compiled lib/demo.ex
Compiled lib/demo/fooable.ex
Compiled lib/demo/foo.ex
Generated demo app
Consolidating ProtocolEx's project-wide...
ProtocolEx beam module Elixir.Demo.Fooable.beam with implementations [Demo.Fooable.DemoFoo]
Consolidating ProtocolEx's project-wide complete.
==> demo_web
Compiling 12 files (.ex)
Compiled lib/demo_web.ex
Compiled lib/demo_web/controllers/error_json.ex
Compiled lib/demo_web/application.ex
Compiled lib/demo_web/telemetry.ex
Compiled lib/demo_web/bar.ex
Compiled lib/demo_web/controllers/page_controller.ex
Compiled lib/demo_web/endpoint.ex
Compiled lib/demo_web/router.ex
Compiled lib/demo_web/components/core_components.ex
Compiled lib/demo_web/controllers/error_html.ex
Compiled lib/demo_web/controllers/page_html.ex
Compiled lib/demo_web/components/layouts.ex
Generated demo_web app
Consolidated Plug.Exception

If the ProtocolEx compiler were getting called at all then it should be working, but it's not getting called. Everything a given implementation needs is in the dependency tree so 'other' apps shouldn't matter.

I'm... like 80% sure this is a mix bug? It's not invoked the compiler in all cases, seems to just do it once and then never again, when it needs to be done for each app that requests it...

DaTrader commented 4 months ago

@josevalim what do you say on this?

On Thu, May 23, 2024, 23:39 OvermindDL1 @.***> wrote:

Looks like because 'demo' is comprehensive and 'demo_web' isn't doing much, hmm...

If I remove the protocol_ex compile from 'demo' so it's just in demo_web since demo_web contains demo then it does indeed compile there:

==> demo_webCompiling 12 files (.ex) Compiled lib/demo_web/application.exCompiled lib/demo_web/controllers/error_json.exCompiled lib/demo_web.exCompiled lib/demo_web/telemetry.exCompiled lib/demo_web/bar.exCompiled lib/demo_web/controllers/page_controller.exCompiled lib/demo_web/endpoint.exCompiled lib/demo_web/router.exCompiled lib/demo_web/components/core_components.exCompiled lib/demo_web/controllers/error_html.exCompiled lib/demo_web/controllers/page_html.exCompiled lib/demo_web/components/layouts.exGenerated demo_web appConsolidating ProtocolEx's project-wide...ProtocolEx beam module Elixir.Demo.Fooable.beam with implementations [Demo.Fooable.DemoWebBar, Demo.Fooable.DemoFoo]Consolidating ProtocolEx's project-wide complete.Consolidated Plug.Exception

But if the compiler is in both then it gets invoked on demo, but not demo_web:

==> demo Compiling 4 files (.ex) Compiled lib/demo/application.ex Compiled lib/demo.ex Compiled lib/demo/fooable.ex Compiled lib/demo/foo.ex Generated demo app Consolidating ProtocolEx's project-wide... ProtocolEx beam module Elixir.Demo.Fooable.beam with implementations [Demo.Fooable.DemoFoo] Consolidating ProtocolEx's project-wide complete. ==> demo_web Compiling 12 files (.ex) Compiled lib/demo_web.ex Compiled lib/demo_web/controllers/error_json.ex Compiled lib/demo_web/application.ex Compiled lib/demo_web/telemetry.ex Compiled lib/demo_web/bar.ex Compiled lib/demo_web/controllers/page_controller.ex Compiled lib/demo_web/endpoint.ex Compiled lib/demo_web/router.ex Compiled lib/demo_web/components/core_components.ex Compiled lib/demo_web/controllers/error_html.ex Compiled lib/demo_web/controllers/page_html.ex Compiled lib/demo_web/components/layouts.ex Generated demo_web app Consolidated Plug.Exception

If the ProtocolEx compiler were getting called at all then it should be working, but it's not getting called. Everything a given implementation needs is in the dependency tree so 'other' apps shouldn't matter.

I'm... like 80% sure this is a mix bug? It's not invoked the compiler in all cases, seems to just do it once and then never again, when it needs to be done for each app that requests it...

— Reply to this email directly, view it on GitHub https://github.com/OvermindDL1/protocol_ex/issues/67#issuecomment-2128061474, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHGBEC4JDMOWMG3CLNJKOH3ZDZOYLAVCNFSM6AAAAABHXTHSYOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRYGA3DCNBXGQ . You are receiving this because you authored the thread.Message ID: @.***>

OvermindDL1 commented 4 months ago

I posted this in that other thread as well.

@DaTrader: You could possibly work around this for now by having one app depend on all your others and have the protocol_ex compiler added on 'just' that one instead of all of them even if that app does nothing else?

DaTrader commented 4 months ago

I will try that

On Thu, May 23, 2024, 23:45 OvermindDL1 @.***> wrote:

I posted this in that other thread as well.

@DaTrader https://github.com/DaTrader: You could possibly work around this for now by having one app depend on all your others and have the protocol_ex compiler added on 'just' that one instead of all of them even if that app does nothing else?

— Reply to this email directly, view it on GitHub https://github.com/OvermindDL1/protocol_ex/issues/67#issuecomment-2128069104, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHGBECY6HUBPKOU7R3E6KN3ZDZPOTAVCNFSM6AAAAABHXTHSYOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRYGA3DSMJQGQ . You are receiving this because you were mentioned.Message ID: @.***>

OvermindDL1 commented 4 months ago

So mix will only run a compiler once, no matter how many apps its defined on, which is... entirely contrary to how one thinks it should work... I'm not sure how this could be worked around without some postprocess step (and I already have the single-function-call you can do to do that yourself, it's really easy).

DaTrader commented 1 week ago

@OvermindDL1 Just letting you know that your suggestion to have just one of the umbrella apps depending on all others and having the protocol_ex compiler works, but then there's another issue - recompile in iex fails if anything is changed in any of those other apps. One needs to exit iex and compile with mix instead.

OvermindDL1 commented 1 week ago

Thanks for confirming in your setup!

Yeah I know, I don't know how to fix these myriad of issues. Who would have ever expected the build system to change in such a backwards incompatible way without a major version bump... ^.^;

Really though, unsure how to fix it without, just... forcing runtime code generation or something. Which teeeechnically you can do in your own project (that's actually why I left those calls public, see the tests, there's a call you can do to pass in the protocol and implementations and it builds and compiles the code at runtime, you can do that on server startup, for IEX you'd just re-call that again right before you 'recompile' maybe? Or maybe after?).

But, again, backwards incompatible changes (documented or not) should never be done without major version bumps for a language tool... >.>

If someone has some weird idea or so to get it working again, please PR! I really can't see how since mix no longer calls third party compilers once per set of source code and rather only just once on whatever random source code it happened to do first (which obviously would break horribly on dependencies using it, still unsure why it was done...).

I guess potentially you could walk the entire path of the everything, but you'd need to figure out the 'parent' projects being compiled (which I don't think that information is even available at that time yet, a major issue...), which dependencies they use, compile the 'sets' of them that each use (you cannot build just one big one for all because dependencies that are not shared across them all would make the code not loadable at runtime), etc... and a few other issues... :-(

OvermindDL1 commented 1 week ago

(as an aside, I still think this functionality should be built into the stock elixir protocol implementation, it would fit in so very well... and completely obviate the need for this)

DaTrader commented 1 week ago

Yeah, me too.