bugmake - Bugs: bug #59956, Recipes inside conditionals can...

 
 

bug #59956: Recipes inside conditionals can break the parser

Submitted by:  None
Submitted on:  Wed 27 Jan 2021 08:29:00 PM UTC
 
Severity:  3 - Normal Item Group:  Bug
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Component Version:  4.3 Operating System:  Any
Fixed Release:  None Triage Status:  None
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

( Jump to the original submission )

Thu 28 Jan 2021 05:23:52 PM UTC, comment #10: 

Using otherwise means that now if a recipe runs a program named otherwise, it will start to fail whereas before it succeeded.

I'm really not jazzed about creating new esoteric syntax for this problem.  Anyway, adding new syntax doesn't really help IMO.  There are already ways to work around this problem by modifying the makefile: we don't need to create anything new for that.  The hope is to find a way to make the current syntax work "as expected", at least in the large majority of cases, without requiring people to rewrite makefiles.

> Being able to indent the conditionals would be a big plus.


You CAN indent conditionals.  You just CAN'T indent them with the .RECIPEPREFIX (TAB by default).

My feeling is that this is entirely reasonable.  Any attempt to allow TAB for generic indentation by making a distinction between a recipe context and not a recipe context is doomed to failure and future pain, and this has been true ever since ifeq was invented.

If you really need to use TAB for indentation then you should modify your makefiles to use a different .RECIPEPREFIX value.  Anything else is madness, or at least leads that way.

Anyway that's my $0.02.

Paul D. Smith <psmith>
Project Administrator
Thu 28 Jan 2021 03:54:28 PM UTC, comment #9: 

What about mixing in the special $ character to help with differentiation? Or adding [] to denote make only syntax.

$[ifeq (1,0)]
    $[info Maybe this could work with leading tabs also]
$[else]
    $[info Or with whitespaces]
$[endif]

Being able to indent the conditionals would be a big plus.

Bogdan V <bogdan>
Thu 28 Jan 2021 01:10:08 PM UTC, comment #8: 

i mean, the user would tell make through some option (a special target or even presence of ".else" in the makefile) "this makefile uses .else, rather then else". make then would not consider "else" a keyword.
The keyword does not have to be ".else". It could be e.g. "otherwise". With "otherwise" there is no need to prefix all conditionals with a ".".

Dmitry Goncharov <dgoncharov>
Thu 28 Jan 2021 06:08:42 AM UTC, comment #7: 

Adding a new keyword doesn't help: it's not that we're trying to use a make else clause but the else is being interpreted as something else, where replacing it with .else would help clarify what is meant; the problem is that we're trying to use a shell (in this situation) else clause, so we can't change it to .else, but make is interpreting it as a make else clause.

The only way new keywords could help would be something like: allowing all make conditional statements (*ifeq*, ifneq, ifdef, else, endif) to be prefixed by ".", and whichever form is used by the if-statement would have to be used throughout.  So if you used .ifeq instead of ifeq then you must also use .else and .endif and else and endif would not be recognized for that if-statement.  This is a lot of churn; I'm not convinced.

I'm not sure what to say about people using TAB to indent conditionals.  That's SO dangerous and SO error-prone!  All that has to happen is that the makefile is modified so these exist in a recipe context and boom!  Your entire makefile is broken.  And how easy is it to create a recipe context?  As I show in my replies below, very easy; here's a simple example:

ifeq (1,1)
        X = true
else
        X = false
endif

all:;@echo $X

That prints true as you expect.  Now if I change this makefile just slightly:

.PHONY: all

ifeq (1,1)
        X = true
else
        X = false
endif

all:;@echo $X

Now X is not set at all!!  How many hours of staring at this makefile will it take to understand what the problem is here?!?!

It's very hard for me to justify preserving such dangerous behavior as a "feature".

An alternative heuristic would be that the same indentation level as the ifeq would be required for all the conditional statements but I suspect that would lead to even more widespread backward-compatibility problems.

Paul D. Smith <psmith>
Project Administrator
Thu 28 Jan 2021 04:20:32 AM UTC, comment #6: 

What about adding another keyword, e.g. .else?

Dmitry Goncharov <dgoncharov>
Thu 28 Jan 2021 02:51:45 AM UTC, comment #5: 

> We can "fix" it by adding a simplifying heuristic; perhaps something like: "conditional statements are only recognized if they are not prefixed by the recipe prefix in effect when the if-statement started".  That is not perfect but it's probably good enough.


I think this heuristic as stated would be disastrous at my $dayjob and probably many others. Almost everyone who works on makefiles here is a C programmer by trade and they're accustomed to indenting by tabs so I see a lot of:

if ...
<tab>if ...
<tab>else
<tab>endif
else
endif

I try to discourage this and have written up a style guide which includes "tabs should be used only as a recipe prefix" but there are still many such instances.

Anonymous
Thu 28 Jan 2021 12:32:26 AM UTC, comment #4: 

Any target can have a recipe, even special targets like .PHONY:, .POSIX:, .ONESHELL:, etc.  Those recipes are ignored but they are syntactically legal.  I guess there would be value in mentioning them if some sort of "warning mode" were enabled.

Re .RECIPEPREFIX: I see; yes I should have tried it myself then I would have realized what you meant.  Right, this bug is a weird confluence of the Original Sin of makefiles (using TAB, which is just whitespace, as a recipe prefix) and the choice in GNU make to use simple else as the keyword; other make implementations use special keywords like .else or similar which are less likely to conflict with "real rules".

If the recipe prefix is anything other than whitespace this bug cannot happen.

Paul D. Smith <psmith>
Project Administrator
Wed 27 Jan 2021 11:24:50 PM UTC, comment #3: 

Thanks for the even more detailed explanation than on Stack Overflow :)
I didn't realize special targets like .ONESHELL: can also have a recipe.

As a clarification, when I mentioned .RECIPEPREFIX: I was also implying actually using it in the recipe:

.ONESHELL:
.RECIPEPREFIX = >
ifeq (1,0)
test:
>@if [ "asd" == "123" ]; then
>        echo "true"
>else
>        echo "false"
>fi
endif

I guess >else is not seen as a valid token and that's why it works.

Bogdan V <bogdan>
Wed 27 Jan 2021 09:36:32 PM UTC, comment #2: 

It may seem trivial but in reality it's pretty hard to hit.  There is essentially only one way it can happen (in valid make recipes), and that's by using .ONESHELL:.

If you don't use .ONESHELL: then the only way it can happen is if you have a program named else that you want to run, which no sane person would ever do, but even if you did want to do that the shell will consider the else to be a keyword; so you'd have to write ./else or something and then make wouldn't match it either.

If you do use .ONESHELL: then you need to have the entire recipe in a make if block with else as its own token on the recipe line.  Using .ONESHELL: is still not that common and putting entire rules inside an if block is also not that common.  ifeq etc. are more commonly used around variable assignments or maybe include files.

Paul D. Smith <psmith>
Project Administrator
Wed 27 Jan 2021 09:26:00 PM UTC, comment #1: 

There is one real bug here, but everything else is just an understandable consequence of that same bug.

The real bug is that the "else" that is part of a recipe, is being interpreted by make as an "else" match for the ifeq.

This is because while make is parsing the "not-taken" side of the if-statement, it is not interpreting the content it sees.  Because it's not interpreting that content, it doesn't realize that the "else" here is indented by a TAB in the context of a rule and it assumes that the "else" is supposed to be attached to the "ifeq".

Fixing this without some heuristic is basically impossible.  In order to allow this to work, make would need to be tracking enough of the "not-taken" side of the if-statement to know whether the "else" is in the context of a recipe or not.  But, people often use "ifeq" to disable entire sections of a makefile, some of which might not even be valid make syntax.  Also consider if the .RECIPEPREFIX was modified inside of the ifeq statement, to something else...!!

We can "fix" it by adding a simplifying heuristic; perhaps something like: "conditional statements are only recognized if they are not prefixed by the recipe prefix in effect when the if-statement started".  That is not perfect but it's probably good enough.

All the other items mentioned here are not really bugs; if you remove the lines starting with ifeq through else (and the endif of course) and just look at the resulting makefile, which is what make is seeing, it's pretty obvious why you get the behavior you do in all cases.

Adding .ONESHELL: makes it "work" because the part of the if-statement after the else is a interpreted as a recipe attached to .ONESHELL: which is not an error although it is a no-op.  In other words this:

.ONESHELL:
ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif

is equivalent to writing this:

.ONESHELL:
                echo "false"
        fi

which is a valid makefile, because we allow any target including a special target like .ONESHELL: to have a recipe; obviously the recipe is invalid shell syntax but that doesn't matter since the recipe is never run.

When you add a variable assignment after the .ONESHELL: and before the ifeq, now you see this:

.ONESHELL:
aaa = 1
                echo "false"
        fi

which is now clearly an invalid makefile.  And if you add another .ONESHELL: you'll get this:

.ONESHELL:
aaa = 1
.ONESHELL:
                echo "false"
        fi

which is, again, just fine.

If you add a character after else, then it's no longer a valid else command for make's ifeq, so it's ignored.  In make conditionals are grouped by word (whitespace separated) so else: is considered a single token which does not equal else, it's not considered two tokens, an else and a :.

If you add a space after the else before more text, then it IS a valid else command, and you'll get an error that there's extraneous text after the else.

Finally, adding .RECIPEPREFIX doesn't really fix anything.  It only changes the error you get.  If you write:

.ONESHELL:
.RECIPEPREFIX = >
ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif

then the makefile make sees is:

.ONESHELL:
.RECIPEPREFIX = >
                echo "false"
        fi

You won't get an error about recipe commencing before first target because the echo line is not a recipe; it doesn't begin with the .RECIPEPREFIX character.  Instead you'll get a missing separator error because these lines are not valid make directives.

Paul D. Smith <psmith>
Project Administrator
Wed 27 Jan 2021 08:29:00 PM UTC, original submission:  

I stumbled across a strange behavior and it's scary how trivial it appears. I tested this with make 4.1, 4.2 and 4.3 on native Ubuntu 20.04, WSL-Ubuntu 18.04 and MSYS2.
Consider the following example:

ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif

The else recipe at line 5 is treated as part of the ifeq (1,0) conditional thus making the next recipe line fail because there is no target before it:

Makefile:6: *** recipe commences before first target.  Stop.

I know the recipe does not make any sense without .ONESHELL, but it should not matter. Maybe the user has a shell with a valid "else" command.

 

Adding .ONESHELL: above (below does not work) fixes the problem. However, if there is any other statement between .ONESHELL: and the conditional, this causes the same problem.
This can be fixed by adding an additional .ONESHELL: just before the conditional:

.ONESHELL: # Adding this line fixes the parser
aaa = 1    # Adding this line breaks the parser again
.ONESHELL: # Adding this line fixes the parser again

ifeq (1,0)
test:
        @if [ "asd" == "123" ]; then
                echo "true"
        else
                echo "false"
        fi
endif

 

If you can add a character immediately after the else, this does not happen anymore:

SHELL = python3
.ONESHELL:

aaa = 1

ifeq (1,0)

test:
        @if 'a' == 'b':
          print('true')
        else: # No space between "else" and ":"
          print('false')

endif

Leaving a whitespace between else and : (which is valid in python) brings the error back.

I found that changing .RECIPEPREFIX: also fixes the problem.

Is this really a bug or just some hidden intended behavior?

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dgoncharov (Posted a comment)
  • -email is unavailable- added by bogdan (Posted a comment)
  • -email is unavailable- added by psmith (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    No changes have been made to this item

    Back to the top


    Powered by Savane 3.9