bugmake - Bugs: bug #63821, Switch -r fails to disable default...

 
 

bug #63821: Switch -r fails to disable default suffix rules.

Submitter:  Dmitry Goncharov <dgoncharov>
Submitted:  Mon 20 Feb 2023 05:52:08 PM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Component Version:  4.3 Operating System:  Any
Fixed Release:  4.4.1 Triage Status:  Medium Effort
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 21 Feb 2023 05:37:49 PM UTC, comment #8: 

Yes I'm going to go through and remove them all as a cleanup, not until after this release though.

Paul D. Smith <psmith>
Group administrator
Tue 21 Feb 2023 04:50:46 PM UTC, comment #7: 


> We can reconsider it.


There is no need. It is all good.

I see that you removed parameter void from install_default_suffix_rules definiton. Going forward, do you want   empty paranthesis rather than void in functions definition?

Dmitry Goncharov <dgoncharov>
Tue 21 Feb 2023 05:02:47 AM UTC, comment #6: 

Hm, I didn't see your reply before I pushed my changes.  We can reconsider it.

I see what you're saying, it's just that I always consider that all rules should be defined before we snap_deps(): once snap_deps() happens we're not allowed to manipulate the DAG (other than adding implicit rule matches).

Probably it doesn't matter either way in this case.

Paul D. Smith <psmith>
Group administrator
Tue 21 Feb 2023 03:00:20 AM UTC, comment #5: 


> The title is slightly misleading


Hopefully, subsequent description made it clear.

> Curious why you decided to install the default suffix rules after snap_deps() @dgoncharov .


i wanted to install default suffix rules as close to convert_to_pattern as possible. i think, it makes it easier to follow the logic in main. install_default_suffix_rules, convert_to_pattern, install_default_implicit_rules all belong together.

>  That doesn't feel right to me,


Are you thinking about the prior impl of snap_deps? We modified snap_deps and removed that code that expanded prerequisites of .SUFFIXES. See 07eea3aa496184bb763b7306e9de6a40a94605c9.
Or do you see a use case that requires install_default_suffix_rules before snap_deps?

> was there something that didn't work right if you did it between defining the makeflags and snap_deps(), rather than after?


No. Either way it works the same, as far as i can see.

Dmitry Goncharov <dgoncharov>
Mon 20 Feb 2023 08:08:49 PM UTC, comment #4: 

Curious why you decided to install the default suffix rules after snap_deps() @dgoncharov .  That doesn't feel right to me, was there something that didn't work right if you did it between defining the makeflags and snap_deps(), rather than after?

Paul D. Smith <psmith>
Group administrator
Mon 20 Feb 2023 06:50:05 PM UTC, comment #3: 

The title is slightly misleading because MAKEFLAGS += -r DOES disable default suffix rules, it just doesn't remove them if you add back the extensions to the .SUFFIXES target.

See this test from tests/scripts/options/dash-r which passes:


# Make sure we can set it from within the makefile too
run_make_test(q!
COMPILE.c = echo
MAKEFLAGS += -r
!,
              'xxx.o',
              "#MAKE#: *** No rule to make target 'xxx.o'.  Stop.", 512);


Since we don't set .SUFFIXES we get the answer we want.  I do agree that your change is needed though: we shouldn't "recover" the rules by setting .SUFFIXES without redefining them.

Paul D. Smith <psmith>
Group administrator
Mon 20 Feb 2023 06:08:09 PM UTC, comment #2: 

A correction: After parsing makefiles the patch appends the default suffix rules to suffix_file->deps only if there is no user defined rules in the makefile.

Dmitry Goncharov <dgoncharov>
Mon 20 Feb 2023 06:02:44 PM UTC, comment #1: 

Setting -r in MAKEFLAGS in makefile fails to the disable default suffix rules.


$ ls
hello.c  makefile
$ cat makefile
MAKEFLAGS+=-r
.SUFFIXES: .c .o
all: hello.o
$ make-4.4
cc    -c -o hello.o hello.c
$
$ rm hello.o
$ make-4.4 -r
make-4.4: *** No rule to make target 'hello.o', needed by 'all'.  Stop.
$


This happens, because make install default suffix rules before parsing makefiles. Once the files are added they stay in hash table "files" and also in suffix_file->deps. Make then creates pattern rules from them.
The patch moves setup of default suffix rules after parsing makefiles. The patch also prepends suffix rules defined in the makefile before the default suffix rules in suffix_file->deps in order to give user defined rules precedence over the default rules.

This ensures that when built-in rules are disabled in the makefile, but suffixes are added to .SUFFIXES, make exits with the 'No rule...' error message. Just like when -r is specified on the command line.
This also relieves make from creating and then destroying file objects for default suffix rules and keeps hash table "files" free of the related entries when built-in rules are disabled in the makefile.

Dmitry Goncharov <dgoncharov>
Mon 20 Feb 2023 05:52:08 PM UTC, original submission:  

.

Dmitry Goncharov <dgoncharov>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54386:  sv63821_fix.diff added by dgoncharov (3KiB - text/x-patch)
file #54387:  sv63821_test.diff added by dgoncharov (3KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-02-21 psmith StatusNone Fixed
        Open/ClosedOpen Closed
        Component Version4.4.1 4.3
        Fixed ReleaseNone 4.4.1
        Triage StatusNone Medium Effort
    2023-02-20 dgoncharov Attached File- Added sv63821_fix.diff, #54386
        Attached File- Added sv63821_test.diff, #54387

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code