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

Makefile.in: Export $MAKE to be used by other scripts #9164

Closed
wants to merge 2 commits into from

Conversation

dumbbell
Copy link
Contributor

@dumbbell dumbbell commented Dec 9, 2024

Why

GNU make is sometimes installed as gmake and make points to another implementation that does not support GNU make extensions.

The name of the make command is stored in $(MAKE) and sub-make processes automatically inherit and use this variable. However other executed commands and scripts do not get this variable by default. Therefore if a script tries to call make(1) again, it will either not find this value or, more likely, will hard-code make.

How

The top-level Makefile now exports the value of $(MAKE) into the environment of all executed sub-shells. This way, scripts can use the correct make command name if they need to run make(1) again.

An example fixed in this patch is make/test_target_script.sh. It used to hard-code make and now uses $MAKE passed from the parent make instance.

[Why]
GNU make is sometimes installed as `gmake` and `make` points to another
implementation that does not support GNU make extensions.

The name of the make command is stored in `$(MAKE)` and sub-make
processes automatically inherit and use this variable. However other
executed commands and scripts do not get this variable by default.
Therefore of a script tries to call make(1) again, it will either not
find this value or, more likely, will hard-code `make`.

[How]
The top-level Makefile now exports the value of `$(MAKE)` into the
environment of all executed sub-shells. This way, scripts can use the
correct make command name if they need to run make(1) again.
... instead of always using the hard-coded `make` command name.

[Why]
This is useful if GNU make is installed under another name (e.g.
`gmake`).

[How]
The correct name is passed through the `$MAKE` environment variable by
the parent make instance: use it.

The script defaults to `make` if the variable is unset.
Copy link
Contributor

github-actions bot commented Dec 9, 2024

CT Test Results

    3 files    143 suites   48m 58s ⏱️
1 596 tests 1 547 ✅ 49 💤 0 ❌
2 340 runs  2 266 ✅ 74 💤 0 ❌

Results for commit 6463837.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@dumbbell dumbbell changed the title Makefile.in: Export $MAKE to be used by other scripts Makefile.in: Export $MAKE to be used by other scripts Dec 9, 2024
@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Dec 16, 2024
@garazdawi garazdawi self-assigned this Dec 16, 2024
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello!

This looks good, though I don't think the export in Makefile.in is needed as we pass MAKE=${MAKE) to test_target when we call it. Or have you found someplace where we do not?

@dumbbell
Copy link
Contributor Author

dumbbell commented Dec 16, 2024

I didn’t give an example in the commit message: GNU make is installed as gmake on FreeBSD which I’m using. I couldn’t run the tests for other pull requests I submitted without this patch because make/test_target_script.sh would call make which would fail to parse Makefiles.

@dumbbell
Copy link
Contributor Author

dumbbell commented Dec 16, 2024

Or have you found someplace where we do not?

I admit, I didn’t try to run the tests without the whole change, @garazdawi. Let me try with just the patch to make/test_target_script.sh.

Edit: If %(MAKE) is not exported, the script doesn’t inherit its value:

erlang/otp  pass-MAKE-var-to-scripts ► gmake stdlib_test ARGS="-suite argparse_SUITE"
 MAKE	stdlib_build
gmake[1]: Entering directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib'
=== Entering application stdlib
gmake[2]: Entering directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/src'
 ERLC	../ebin/argparse.beam
 ERLC	../ebin/shell_docs.beam
 VSN	../ebin/stdlib.app
 VSN	../ebin/stdlib.appup
gmake[2]: Leaving directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/src'
gmake[2]: Entering directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/doc'
gmake[2]: Nothing to be done for 'opt'.
gmake[2]: Leaving directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/doc'
gmake[2]: Entering directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/examples'
gmake[2]: Nothing to be done for 'opt'.
gmake[2]: Leaving directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/examples'
=== Leaving application stdlib
gmake[1]: Leaving directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib'
ERL_TOP=/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp TYPE= gmake -C lib/stdlib test
gmake[1]: Entering directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib'
gmake TEST_NEEDS_RELEASE=true \
	ERL_ARGS="-enable-feature maybe_expr "    \
	-f /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/make/app_targets.mk test
gmake[2]: Entering directory '/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib'
TEST_NEEDS_RELEASE=true TYPE= \
  /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/make/test_target_script.sh /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp

...

======================================================================

"make release RELEASE_ROOT=/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/make_test_dir/Erlang ∅⊤℞" failed.\nSee /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/lib/stdlib/make_test_dir/release_tests_log for full logs

======================================================================
make[3]: "/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/Makefile" line 564: Invalid line 'ifeq ($(TARGET),win32)', expanded to 'ifeq '
	in directory /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp
make[3]: "/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/Makefile" line 588: Invalid line 'else'
	in directory /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp
make[3]: "/home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/Makefile" line 616: Invalid line 'endif'
	in directory /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp
...

So the following command should set MAKE on the command line instead?

gmake TEST_NEEDS_RELEASE=true \
	ERL_ARGS="-enable-feature maybe_expr "    \
	-f /home/dumbbell/Documents/Dev/rabbitmq/erlang/otp/make/app_targets.mk test

@dumbbell
Copy link
Contributor Author

Here is another proposal:

diff --git a/Makefile.in b/Makefile.in
index aaad4ba93e..f2948aa4ad 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -477,11 +473,11 @@ PROFILE_EMU_DEPS=
 endif
 
 emulator_test: emulator
-	$(make_verbose)cd erts/emulator && ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) test
+	$(make_verbose)cd erts/emulator && ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) MAKE=$(MAKE) test
 epmd_test:
-	$(make_verbose)cd erts/epmd && ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) test
+	$(make_verbose)cd erts/epmd && ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) MAKE=$(MAKE) test
 system_test:
-	$(make_verbose)cd erts && ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) test
+	$(make_verbose)cd erts && ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) MAKE=$(MAKE) test
 
 emulator: $(PROFILE_EMU_DEPS)
 	$(make_verbose)cd erts && ERL_TOP=$(ERL_TOP) PATH=$(BOOT_PREFIX)"$${PATH}" \
@@ -992,4 +988,4 @@ APPS_TEST=$(patsubst %, %_test,$(APPS))
 .SECONDEXPANSION:
 $(NO_DIALYZER_APPS): $$(patsubst %,%_build,$$@)
 $(APPS_TEST): $$(patsubst %_test,%_build,$$@)
-	ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) -C lib/$(patsubst %_test,%,$@) test
+	ERL_TOP=$(ERL_TOP) TYPE=$(TYPE) $(MAKE) -C lib/$(patsubst %_test,%,$@) MAKE=$(MAKE) test

@garazdawi
Copy link
Contributor

You need #8888 for it to work I think. Did you run with that?

@dumbbell
Copy link
Contributor Author

Oh, I already submitted a patch against main in the past and forgot about it while working on maint… I have a memory like a goldfish… Indeed, it works after cherry-picking the patch from #8888.

I apologize for wasting your time.

@dumbbell dumbbell closed this Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants