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

Add CI #2

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Add CI #2

wants to merge 6 commits into from

Conversation

fishinthecalculator
Copy link
Contributor

No description provided.

@mayel
Copy link
Member

mayel commented Oct 5, 2024

curious why Mixer is needed here? I thought it was only used in the parent app?

@fishinthecalculator
Copy link
Contributor Author

I'm not sure why it is used but it is used for configuring bonfire , see https://github.com/bonfire-networks/bonfire/actions/runs/11188229874/job/31106656681

@mayel
Copy link
Member

mayel commented Oct 8, 2024

but isn't that triggered because you copied the mess.exs from bonfire-app rather than the simpler one used in extensions?

@fishinthecalculator
Copy link
Contributor Author

ah that may very well be, let me try to fix this thank you for bringing that up. anyway I think this is yet another reason to make mess a configurable full fledged library with a single source of truth codebase that can serve the needs of the umbrella app and extensions. the chicken and egg problem must be solved the same way it has been done for mix, since you can't install mix from mix itself. but yes I agree this is probably more complicated than valuable. I'd like to come up with some solution for this because it makes contributing deeper to extensions more complex in my experience.

@fishinthecalculator
Copy link
Contributor Author

anyway it seems it doesn't depend on simpler/more complex mess but on an actual call to Bonfire.Mixer https://github.com/bonfire-networks/bonfire/blob/main/config/bonfire_data.exs#L37

@mayel
Copy link
Member

mayel commented Oct 8, 2024

anyway it seems it doesn't depend on simpler/more complex mess but on an actual call to Bonfire.Mixer main/config/bonfire_data.exs#L37

ah, I moved that config file out of the parent app recently and didn't realise that, it's an easy fix though, for example copying these small functions to Bonfire.Common.Extend:


    def deps_tree do
      if function_exported?(Mix.Project, :deps_tree, 0) do
        Mix.Project.deps_tree()
      end
    end

    def deps_tree_flat(tree \\ deps_tree())

    def deps_tree_flat(tree) when is_map(tree) do
      # note that you should call the compile-time cached list in Bonfire.Application
      (Map.values(tree) ++ Map.keys(tree))
      |> List.flatten()
      |> Enum.uniq()
    end

    def deps_tree_flat(_), do: nil

@fishinthecalculator
Copy link
Contributor Author

I just tried changing from Bonfire.Mixer.deps_tree_flat to Bonfire.Common.Extend.deps_tree_flat but I have the same problem since to be run it first has to fetch bonfire_common but to fetch it, it needs to evaluate config/bonfire_data.exs.

** (UndefinedFunctionError) function Bonfire.Common.Extend.deps_tree_flat/0 is undefined (module Bonfire.Common.Extend is not available)
    Bonfire.Common.Extend.deps_tree_flat()
    /home/paul/code/bonfire/bonfire/config/bonfire_data.exs:37: (file)
    (stdlib 6.1.1) erl_eval.erl:904: :erl_eval.do_apply/7
    (stdlib 6.1.1) erl_eval.erl:479: :erl_eval.expr/6
    (stdlib 6.1.1) erl_eval.erl:648: :erl_eval.expr/6

Does it make sense to change:

known_deps = Bonfire.Common.Extend.deps_tree_flat() || []

to:

known_deps = if Code.ensure_loaded?(Bonfire.Common.Extend) , do: Bonfire.Common.Extend.deps_tree_flat() || [], else: []

It is kind of hacky because all mix commands run before running mix deps.get would have maybe_extension_schema behave differently than it would with bonfire_common installed but I'm not sure there is another way without refactoring maybe_extension_schema as well. what do you think? thanks for the help :)

@mayel
Copy link
Member

mayel commented Oct 10, 2024

Ah yeah that should work! Because before the deps are fetched that list wouldn't contain them anyway, so this doesn't alter the behaviour in any meaningful way :)

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 this pull request may close these issues.

2 participants