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

Fix MAKEFLAGS parsing with Make 4.4+ #395

Closed
wants to merge 1 commit into from

Conversation

minijackson
Copy link
Contributor

Since Make version 4.4, MAKEFLAGS also contains long options and overridden variables on the command-line (source).

This means that parsing by filtering out --% doesn't work reliably anymore, since it doesn't remove overrides:

Running make VAR=quacks gives MAKEFLAGS=" -- VAR=quacks", and checkflags would understand that flags -q, -s, ... were set.

This would get transmitted below into QUIET_FLAGS and QUESTION_FLAG, then passed to the genVersionHeader.pl as -i and -q.

The result would be that genVersionHeader.pl would never create the version header (only check for its up-to-date status), leading to confusing errors:

  ../misc/epicsRelease.c:25:32: error: expected ')' before 'EPICS_VCS_VERSION'
     25 |     printf ( "## %s\n", "Rev. " EPICS_VCS_VERSION );
        |            ~                   ^~~~~~~~~~~~~~~~~~

The NEWS file recommends using firstword, so we can use just that.

@mdavidsaver
Copy link
Member

A good catch.

Do you (and @anjohnson) think any of the other WARNING: Backward-incompatibility! items in the linked NEWS file will effect us?

At a glance, I think not. With the possible exception of the change to $(shell and exported environment.

@simon-ess
Copy link
Collaborator

Unless I'm misunderstanding something, this no longer does the same thing for older versions of make: If I run make with a mix of flags (say -q -n --no-print-directory) then I see the following sort of output:

$(info ${MAKEFLAGS})
$(info $(filter-out --%,$(MAKEFLAGS)))
$(info $(firstword $(MAKEFLAGS)))

yielding

 --no-print-directory -sqn
-sqn
--no-print-directory

as such the change you propose seems to break things for at least make 3.82

@minijackson
Copy link
Contributor Author

@simon-ess aahhh, there goes my simple fix...

You're right, the NEWS file mention that the MAKEFLAGS variable is rigorous only from GNUMake 4.0+ (here)

For a quick check I did:

make -p -q -s --no-print-directory -j4 A=4 | grep MAKEFLAGS

yields for make 4.4.1:

MAKEFLAGS = pqs -j4 --jobserver-auth=fifo:/tmp/GMfifo130336 --no-print-directory -- $(MAKEOVERRIDES)

and for make 3.82:

MAKEFLAGS =  --no-print-directory -sqp --jobserver-fds=3,4 -j -- $(MAKEOVERRIDES)

Is there any possibility of removing this parsing as a whole?

@anjohnson
Copy link
Member

There are discussions about testing MAKEFLAGS in the answers to this question on Stack Overflow, but they probably all pre-date 4.4; My answer describes the solution that's currently in Base and mentions the subtle difference between its in-recipe and outside values. Could someone please try using the test Makefile in my answer on Make 4.4 to confirm that it does get false results.

@simon-ess The NEWS file recommends using $(firstword -$(MAKEFLAGS)) which isn't quite what you tested above (you missed the - sign), although I don't see that it would make any significant difference.

If necessary we might be able to add a version check to continue to support older versions, note that Apple continues to ship GNU Make version 3.81 with MacOS and/or XCode. Later versions were released under the GPLv3 which they will not accept.

I don't want to remove these checks, they are too useful.

I didn't see anything obvious in the NEWS incompatibility descriptions, hopefully this is the only thing that we will have to resolve.

@minijackson
Copy link
Contributor Author

@anjohnson checking your test Makefile:

define checkflags
  make-$1 = $(findstring $1,$(filter-out --%,$(MAKEFLAGS)))
endef
$(foreach flag,i r w s k n q, $(eval $(call checkflags,$(flag))))

$(info MAKEFLAGS = "$(MAKEFLAGS)")
$(foreach flag,i r w s k n q, \
    $(info make-$(flag) = "$(make-$(flag))" $(if $(make-$(flag)),YES,NO)))

all:
    @+echo 'all: MAKEFLAGS = "$(MAKEFLAGS)"'
    @+echo 'all: $(foreach f,i r w s k n q,$(make-$f))'
    @+echo ''

And running make A=irwsknq

with GNUMake 4.4.1:

MAKEFLAGS = " -- A=irwsknq"
make-i = "i" YES
make-r = "r" YES
make-w = "w" YES
make-s = "s" YES
make-k = "k" YES
make-n = "n" YES
make-q = "q" YES
Makefile:13: *** missing separator.  Stop.

With GNUMake 4.3:

MAKEFLAGS = ""
make-i = "" NO
make-r = "" NO
make-w = "" NO
make-s = "" NO
make-k = "" NO
make-n = "" NO
make-q = "" NO
Makefile:13: *** missing separator.  Stop.

GNUMake 3.82 has the same result as 4.3.

@anjohnson
Copy link
Member

This version of checkflags seems to work for me on both 3.81 and 4.4.1:

makeflags := $(firstword $(filter-out -,$(filter-out --%,$(MAKEFLAGS))))
define checkflags
    make-$1 := $(findstring $1,$(makeflags))
endef

3.81 seems to insert the odd - word into MAKEFLAGS, but this combination of filters left just the single letter options in the first word in all my tests.

@minijackson Please adjust your PR and test this.

@AppVeyorBot
Copy link

Since Make version 4.4, MAKEFLAGS also contains long options and
overridden variables on the command-line[1].

[1]: https://git.savannah.gnu.org/cgit/make.git/tree/NEWS?h=4.4#n67

This means that parsing by filtering out '--%' doesn't work reliably
anymore, since it doesn't remove overrides:

Running 'make VAR=quacks' gives 'MAKEFLAGS=" -- VAR=quacks"', and
'checkflags' would understand that flags -q, -s, ... were set.

This would get transmitted below into 'QUIET_FLAGS' and
'QUESTION_FLAG', then passed to the 'genVersionHeader.pl' as '-i' and
'-q'.

The result would be that 'genVersionHeader.pl' would never create the
version header (only check for its up-to-date status), leading to
confusing errors:

  ../misc/epicsRelease.c:25:32: error: expected ')' before 'EPICS_VCS_VERSION'
     25 |     printf ( "## %s\n", "Rev. " EPICS_VCS_VERSION );
        |            ~                   ^~~~~~~~~~~~~~~~~~

The NEWS file[1] recommends using 'firstword', but unfortunately this is
not compatible with GNUMake < 3.82.
@minijackson
Copy link
Contributor Author

@anjohnson done, sorry for the delay.

Tested by compiling EPICS with GNUMake 4.4 and 4.3. Compiling with Make 3.82 is a bit harder for me, I don't know if someone (@simon-ess?) can test it?

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@simon-ess
Copy link
Collaborator

The current version behaves as expected for GNUMake 3.82.

Copy link
Member

@anjohnson anjohnson left a comment

Choose a reason for hiding this comment

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

Thanks @simon-ess, this looks good now.

@anjohnson anjohnson added the Approved Ready To Merge label Jul 5, 2023
@mdavidsaver
Copy link
Member

Applied as d87fd0d

@minijackson minijackson deleted the fix-make-4.4 branch February 1, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Ready To Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants