-
-
Notifications
You must be signed in to change notification settings - Fork 238
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 Elixir.mk - Allows for use of Elixir deps within an Erlang project. #979
base: master
Are you sure you want to change the base?
Conversation
Currently rebasing the change :) |
@essen I've added a test under |
Hello! I've not had a good look yet but I doubt this works with some Elixir NIFs since they often have a Makefile and thus wouldn't enter the elixir autopatch. |
Damn, that's a good shout - any idea of an elixir lib which uses NIFs? |
There's a few in this search but gotta filter. Oftentimes if the page says mix + make as a build tool, that's an Elixir NIF. But not always. |
I've just commited a change which should now make it support Elixir Nifs (assuming they use elixir_make - I'm not 100% familiar with the Elixir NIF ecosystem so not quite sure if I've accounted for the right things there). I've added an additional test which will attempt to compile If you've any more suggestions @essen, I'm more than happy to add them 👍 |
Any chance I could poke you to look this over @essen? 😄 |
core/elixir.mk
Outdated
$(verbose) $(MAKE) --no-print-directory $(PROJECT).d | ||
$(verbose) $(MAKE) --no-print-directory app-build | ||
|
||
app-build: ebin/$(PROJECT).app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be app-build::
.
Does this work even if using local system installed Elixir? |
In what sense @essen? I'm expecting elixir to always be loaded as a dep of the project rather than as an installed version on the system - I can try and account for this though if you'd prefer that functionality to be in there. I'll edit this for confirmation but the only thing I think that could be problematic with an installed version of elixir is if |
I think we should take the dep Elixir if one is available as a dep, and the system Elixir otherwise. Basically the dep can be used as an override. Most projects don't require a specific version of Elixir, and for example in RabbitMQ we explicitly use the system Elixir (we even build packages for it). |
@essen could you have a look over when you have chance? Based on your feedback, I've updated the MR to default to using the System Install of Elixir if available. This can be manually overriden by a user-set var though ( Let me know what you think :) |
just poking you 😄 |
I'll get to it no need to remind so often. |
@essen sorry to poke on this one again, any idea when you'll be able to get round to it? :) |
test/log/console.log
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these log files should be commited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
core/elixir.mk
Outdated
|| git describe --dirty --abbrev=7 --tags --always 2>/dev/null || true)) | ||
$(eval MODULES := $(patsubst %,'%',$(sort $(notdir $(basename \ | ||
$(filter-out $(ELIXIRC_EXCLUDE_PATHS), $(ERL_FILES) $(CORE_FILES) $(BEAM_FILES))))))) | ||
$(eval MODULES := $(call UNIQUE,$(MODULES) $(foreach file, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering why the unique filter is necessary? I don't think modules should have duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a fair point, likely more safety/fear on my part 👍
core/deps.mk
Outdated
@@ -148,7 +148,7 @@ endif | |||
|
|||
# Core targets. | |||
|
|||
ALL_APPS_DIRS_TO_BUILD = $(if $(LOCAL_DEPS_DIRS)$(IS_APP),$(LOCAL_DEPS_DIRS),$(ALL_APPS_DIRS)) | |||
ALL_APPS_DIRS_TO_BUILD = $(if $(filter-out $(ELIXIR_BUILTINS),$(LOCAL_DEPS_DIRS))$(IS_APP),$(filter-out $(ELIXIR_BUILTINS),$(LOCAL_DEPS_DIRS)),$(ALL_APPS_DIRS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Elixir builtins should be in LOCAL_DEPS_DIRS
to begin with. It's defined as:
LOCAL_DEPS_DIRS = $(foreach a,$(LOCAL_DEPS),$(if $(wildcard $(APPS_DIR)/$(a)),$(APPS_DIR)/$(a)))
It's not meant to contain applications the user didn't write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely a misunderstanding on my part - I'd assumed they needed adding to the LOCAL_DEPS_DIRS
var since I was adding them to LOCAL_DEPS
@@ -778,14 +780,16 @@ define dep_fetch_ln | |||
ln -s $(call dep_repo,$(1)) $(DEPS_DIR)/$(call dep_name,$(1)); | |||
endef | |||
|
|||
HEX_REPO = https://repo.hex.pm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not extract that into a variable. We want to use hex_core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only pushback against that would be for if someone is using an internal mirror within a corporate setting - it's likely they'd want to instead have the hex side of erlang.mk
pointing at whatever their mirror is
looking back at it, I think I'd likely want to change this to be
HEX_REPO ?= https://repo.hex.pm
@@ -86,7 +86,7 @@ define app_file | |||
endef | |||
endif | |||
|
|||
app-build: ebin/$(PROJECT).app | |||
app-build:: ebin/$(PROJECT).app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is necessary? I don't think the app-build:: ...
rule in the elixir.mk file does anything more than we do here. It doesn't need to be defined in two places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a fair point, I've also removed the $(PROJECT).d
target & the body of the app::
target above this
core/elixir.mk
Outdated
@@ -0,0 +1,334 @@ | |||
ifeq ($(pkg_elixir_commit),master) | |||
pkg_elixir_commit = main | |||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just change the commit in the package.
$(if $(PROJECT_MOD),{mod$(comma) {'$(PROJECT_MOD)'$(comma) []}}$(comma),) | ||
{env, $(subst \,\\,$(PROJECT_ENV))}$(if $(findstring {,$(PROJECT_APP_EXTRA_KEYS)),$(comma)$(newline)$(tab)$(subst \,\\,$(PROJECT_APP_EXTRA_KEYS)),) | ||
]}. | ||
endef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this was redeclared either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this redeclaration was to have .app files generate where the mod
may actually be an elixir module
Currently, the mod
prop only gets inserted if an erlang file exists.
In efforts to separate erlc.mk
from elixir.mk
, I opted to redefine the macro - if you'd prefer, I could add a condition to the definition in erlc.mk
?
core/elixir.mk
Outdated
erlang:halt(1) | ||
end, | ||
maps:get(version, PackageInfo) | ||
end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be moved to the hex part so we can use it for Erlang deps as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you meant the GetDeps
fun, I've moved that to elixir_get_deps.erl
within elixir.mk - if you meant the macro itself, let me know and I'll move it over 👍
Is the thing that transforms paths like Elixir/MyApp/MyMod.ex into module name |
74a2fa0
to
d141f2c
Compare
@essen had some time recently so will be picking this one back up 👍 |
Compiler magic 🪄 |
I will try to make this work with RabbitMQ CLI which is (currently) in Elixir. Could you rebase to run CI now that I fixed it? Also you will need to edit the workflow to add/change the new suites here https://github.com/ninenines/erlang.mk/blob/master/.github/workflows/ci.yaml#L27 |
@essen rebased, hopefully ci should work as expected 👍 |
I'm looking to merge this now. No need to rebase, I am probably going to cherry-pick parts of the code while I try to make things work. |
This change introduces a new core plugin called
elixir.mk
.This change accomplishes the following:
mix.exs
projects to useerlang.mk
.ex
files, allowing for a project to use either Erlang, Elixir or a combination of both$(PROJECT).app
This change makes use of
hex.mk