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

Disable GNU make implicit variables and Quiet ar with ARFLAGS rather than > null #2711

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ParadoxV5
Copy link
Contributor

@ParadoxV5 ParadoxV5 commented Apr 18, 2024

CC, AR and ARFLAGS (and also RM and CXX) are all implicit variables in GNU make.

  • CC and AR already defaults to cc and ar, which means CC ?= cc does nothing since before I started tinkering.
  • ARFLAGS defaults to -rv, where v stands for verbose.

Rather than adapting the Makefile and extconf.rb around them, I disabled them with --no-builtin-variables. While this flag may be GNU-exclusive, the README implies that we only support GNU make.

  • CURDIR from Avoid shelling *nix commands #2706 is also possibly GNU-exclusive.
  • The popularity of GNU means it’s a-okay to be toolset-specific rather than platform-specific.

Makefile Outdated
@@ -12,7 +12,7 @@ SOEXT ?= $(shell ruby -e 'puts RbConfig::CONFIG["SOEXT"]')

CPPFLAGS := -Iinclude $(CPPFLAGS)
CFLAGS := -g -O2 -std=c99 -Wall -Werror -Wextra -Wpedantic -Wundef -Wconversion -Wno-missing-braces -fPIC -fvisibility=hidden $(CFLAGS)
CC ?= cc
ARFLAGS := -r
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced ?= in #2705 to be explicit that the variable may be overwritten by env vars sent from extconf.rb.
But a quick test shows that <implicit variable> ?= <anything> doesn’t do anything because the variable was already set – implicitly by GNU make! 😅
This is why this assignment must be either = or := but not ?=. It shouldn’t affect receiving the rbconfig ARFLAGS through the aforementioned env vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • TODO: circument only for ar -rv; do not change for libtool (such as used in that one failing CI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to --no-builtin-variables. Explicit is better than implicit anyways.

@@ -36,7 +36,7 @@ build/libprism.$(SOEXT): $(SHARED_OBJECTS)

build/libprism.a: $(STATIC_OBJECTS)
$(ECHO) "building $@"
$(Q) $(AR) $(ARFLAGS) $@ $(STATIC_OBJECTS) $(Q1:0=>/dev/null)
$(Q) $(AR) $(ARFLAGS) $@ $(STATIC_OBJECTS)
Copy link
Contributor Author

@ParadoxV5 ParadoxV5 Apr 18, 2024

Choose a reason for hiding this comment

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

A quick text find shows that this was the only recipe where Q1 is used, and the only other use of Q1 is

Q = $(Q1:0=@)
, which may now be flattenend.
(I’m a novice in C-land; if the entire set of Vs and Qs is a convention, introduce them to me.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this matter, ba82ce9 introduces an acutal use for V0:

prism/Makefile

Line 17 in ba82ce9

ARFLAGS ?= -r$(V0:1=v)

@ParadoxV5
Copy link
Contributor Author

(Mac Ruby 2.7 CI failure)

Intriguing.
CC and co. should be set from the rbconfig in practice. Considering this was never an issue in my previous Makefile PRs, does this mean that the Mac Ruby 2.7 was set with libtool for AR, nothing for ARFLAGS, and make had a different default for Mac?
I do not know how to investigate a Mac problem.

Easy solution: drop Ruby 2.7 support (we’re still in 0.x aren’t we?)

@kddnewton
Copy link
Collaborator

Unfortunately we cannot drop 2.7 support, as that is BASERUBY and we need it to build Ruby itself.

@kddnewton
Copy link
Collaborator

CC and AR already defaults to cc and ar, so setting CC to cc does nothing since before I started tinkering.

I'm not sure this is true. Before, we always set CC to cc, so it couldn't be overridden by env vars. Now it's going to be the same CC that Ruby is compiled with.

@ParadoxV5
Copy link
Contributor Author

Before, we always set CC to cc, so it couldn't be overridden by env vars.

Like with ARFLAGS in the PR, I expect the original CC := cc to be overridable by env vars.

Now it's going to be the same CC that Ruby is compiled with.

This though is intended.

@ParadoxV5
Copy link
Contributor Author

Like with ARFLAGS in the PR, I expect the original CC := cc to be overridable by env vars.

Oof, I had it mixed up. It’s only overridable with make KEY=VALUE, not from env vars.

@kddnewton
Copy link
Collaborator

Hey @ParadoxV5 are you still working on this or should this be closed?

@ParadoxV5
Copy link
Contributor Author

Hey @ParadoxV5 are you still working on this or should this be closed?

Hey, @kddnewton, now that https://bugs.ruby-lang.org/issues/20499 has properly fixed #2716 in the Ruby upstream, I plan to get back to this these few of days.
By the way, any plans to revert the workaround #2725 (after Ruby releases teenies with the fix)? [c.c. @eregon]

@kddnewton
Copy link
Collaborator

I think I'd prefer to leave it because I'd like to support all of the Ruby versions in the range that we support, which includes patch revisions.

@eregon
Copy link
Member

eregon commented May 25, 2024

Yeah agreed let's keep that workaround for existing releases and link to the relevant issues to make it easy to figure out from the code why that is needed

`ARFLAGS` is an implicit variable in GNU `make` (the `README` _specifies GNU_ `make`) and defaulted verbosely, so un-verbose-fy it is the more direct *and platform-agnostic* way to quiet `ar`.
As observed in ruby#2771, `?=` thinks that implicit definitions of CC`, `AR` and `ARFLAGS` are explicit.
Disabling implicit variables is more straightforward than adapting the `Makefile` and `extconf.rb` around them.
@ParadoxV5 ParadoxV5 marked this pull request as ready for review May 25, 2024 21:00
@ParadoxV5 ParadoxV5 changed the title Quiet ar with ARFLAGS rather than > null & Remove CC ?= cc Disable GNU make implicit variables and Quiet ar with ARFLAGS rather than > null May 25, 2024
@ParadoxV5 ParadoxV5 marked this pull request as draft May 25, 2024 21:57
`File::exist?` does not check the PATH, meaning the checks fail as long as `CC`/`AR` aren’t absolute paths. OTOH, `MakeMakefile#find_executable` does.
@ParadoxV5 ParadoxV5 marked this pull request as ready for review May 25, 2024 22:19
@@ -51,6 +51,7 @@ def make(env, target)
system(
env,
RUBY_PLATFORM.include?("openbsd") ? "gmake" : "make",
"--no-builtin-variables", # don't let GNU make implicit variables override variable fallbacks in the Makefile
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand what this is doing. Could you explain why we need this?

Copy link
Contributor Author

@ParadoxV5 ParadoxV5 May 28, 2024

Choose a reason for hiding this comment

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

As noted on the previous posts, this flag disables implicit variables for GNU make (updated top post).
GNU make takes those implicit variables precedence over our ?=s, especially the new ARFLAGS ?= -r$(V0:1=v) (PR thread), meaning they aren’t applied without this flag.

prism/Makefile

Lines 15 to 17 in 9bb8710

CC ?= cc
AR ?= ar
ARFLAGS ?= -r$(V0:1=v)

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.

None yet

3 participants