bugmake - Bugs: bug #60077, Deterministic $@ for grouped...

 
 

bug #60077: Deterministic $@ for grouped targets patch

Submitter:  Todd Lowe <toddlowe>
Submitted:  Thu 18 Feb 2021 06:42:02 PM UTC
   
 
Severity:  3 - Normal Item Group:  Enhancement
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Component Version:  4.3 Operating System:  None
Fixed Release:  None Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 22 Jun 2021 01:10:22 AM UTC, comment #7: 

I wonder if &: makes sense as a pattern rule since regular pattern rules already support multiple targets. 

Regardless, I have also tested your implicit test without the & and get the same results:

$ cat ../makefile4
.SECONDEXPANSION:

all: world.z hello.x

%.x %.z: $$(info in prereqs @ = $$@, @< = $$(@<), @^ = $$(@^))
        $(info in recipe @ = $@, @< = $(@<), @^ = $(@^))

$ ./make -f ../makefile4 hello.x
in prereqs @ = hello.x, @< = hello.x, @^ = hello.x
in recipe @ = hello.x, @< = hello.x, @^ = hello.x hello.z
make: 'hello.x' is up to date.

I also tried hello.z and $(@<) is only correct inside the recipe as well.
$ ./make -f ../makefile4 hello.z
in prereqs @ = hello.z, @< = hello.z, @^ = hello.z
in recipe @ = hello.z, @< = hello.x, @^ = hello.x hello.z
make: 'hello.z' is up to date.

The problem appears to be due to second expansion for pattern rules happening quite some time before also_make is set in pattern_search().

Todd Lowe <toddlowe>
Fri 11 Jun 2021 03:33:53 PM UTC, comment #6: 

Thank you for your feedback Dmitry. 

I'm going to start sounding like a broken record here, but again I'm not sure when I will be able to look at this.  If I get a chance before anyone else picks it up, I will debug using your test case and post an update.

Todd Lowe <toddlowe>
Wed 09 Jun 2021 08:37:24 PM UTC, comment #5: 

Todd, thank for your contribution.

Your patch is missing second expansion tests.

i observe the following misbehavior.


$ cat makefile
.SECONDEXPANSION:

hello world&: $$(info in prereqs @ = $$@, @< = $$(@<), @^ = $$(@^))
        $(info in recipe @ = $@, @< = $(@<), @^ = $(@^))
$ ~/src/gmake/make/l64/make -f makefile hello
in prereqs @ = world, @< = hello, @^ = hello world
in prereqs @ = hello, @< = hello, @^ = hello world
in recipe @ = hello, @< = hello, @^ = hello world
make: 'hello' is up to date.
$
$
$ cat makefile4
.SECONDEXPANSION:

all: world.z hello.x

%.x %.z&: $$(info in prereqs @ = $$@, @< = $$(@<), @^ = $$(@^))
        $(info in recipe @ = $@, @< = $(@<), @^ = $(@^))
$ ~/src/gmake/make/l64/make -f makefile4 hello.x
in prereqs @ = hello.x, @< = hello.x, @^ = hello.x
in recipe @ = hello.x, @< = hello.x, @^ = hello.x hello.z
make: 'hello.x' is up to date.


Explicit rules are good.
However, you can see during the 2nd expansion of implicit rules @^ expands to the 1st target, rather than the whole group.

Dmitry Goncharov <dgoncharov>
Thu 13 May 2021 09:10:21 PM UTC, comment #4: 

I've had a bit more time and am uploading a patch with my latest version.  Unfortunately I'm not sure when I'll be able to test it in production, but I wanted to share sooner than later.

This one adds new automatic variables for grouped targets, both explicit and pattern along with new grouped targets and variables test cases.

$(@<) Is the first specified target
$(@^) Is the list of targets in the order specified in the goal definition.
$(@<F), $(@<D), $(@^F), and $(@^D) are also implemented.

My implementation makes two changes to how the also_makes list is stored in order to facilitate the automatic variables.
1 - The stored order now matches the order specified in the rule.
2 - also_makes for pattern rules didn't include the executing target.  This is different than explicit grouped targets where all targets are included in the list.  I have changed this so all targets are always stored in also_makes, but I am concerned there may be side effects of this change.  I do not use pattern rules enough to know, but it does pass the test suite.



(file #51432)

Todd Lowe <toddlowe>
Tue 11 May 2021 08:40:06 PM UTC, comment #3: 

I have a patch that introduces $(@<) as the first target and $(@^) as the list, along with D and F versions of them.

I have not yet added a full suite of tests, and $(@^) is currently in reverse order due to it simply following the internal also_make linked list.

Would you like this partial patch?

Todd Lowe <toddlowe>
Mon 08 Mar 2021 04:03:29 PM UTC, comment #2: 

I admit I wasn't too thrilled about making $@ behave differently between explicit and pattern rules, but I definitely did not want to change the long standing pattern rule behaviour and didn't find the grouped-target $@ useful.  I had proposed $! for instigating target, but I can see that the inconsistency in that would be worse than the solution.

I'm not sure why I didn't like the idea of $(firstword $&), but likely because I was too focused on $@ not behaving the way it did with my pseudo grouped-targets by macro :-)

I had not thought about $(@^) and $(@<), but I like the consistency they have with the existing variables.

I wrote my patch last fall and other priorities have delayed our move to 4.3. If I get back to it before someone else, I will implement these and provide a new patch.

Todd Lowe <toddlowe>
Mon 08 Mar 2021 03:30:30 PM UTC, comment #1: 

I'm not too jazzed about the idea of $@ working differently for explicit vs. pattern rule grouped targets.  I agree that the "instigating target" may not be so useful, but if we change $@ to be a deterministic target then there's NO way to obtain the instigating target: that information is lost.

What I would prefer is to leave $@ as it is and implement a new variable containing the list of all grouped targets ($& seems like a good choice to me, or else we could use something like $(@^) which would kind of be like $^ but for targets) for both explicit AND pattern rules, then users can either use $(firstword $&) to obtain the deterministic target or we could introduce a new special variable like $(@<) or something which was basically the same thing.

Paul D. Smith <psmith>
Group administrator
Thu 18 Feb 2021 06:42:02 PM UTC, original submission:  

My team currently uses a macro that expands to multiple rules as a way to simulate a rule with multiple targets.  I would like to use grouped-targets instead, but like others, have discovered that $@ referring to the target that triggered execution rather than a specific target from the list makes it difficult to use.

I have had a look at
[bug #57898] Automatic variable for all targets on grouped target
and
[bug #58845] In would be nice to have "$&" expand to all targets in a grouped target
But they refer to a way to get all targets rather than a deterministic way to always get a “primary target”.

For my use case, I would like $@ to always expand to a specific target from the grouped list, similar to how $< is always the first prereq.  I’m not a fan of non-backwards compatible changes, but due to the way grouped-targets works,  I think this change makes $@ more useful and allows it’s behaviour to be more consistent between regular recipes and grouped-target recipes.

I’ve attached a patch that causes $@ for grouped-targets to always be the first target from the list plus changes to the test suite for the new behaviour.

Matching the prereq behaviour of having variables for first, trigger, and all seems cleaner than requiring recipes to extract a target from the entire list so I am also considered adding $& for all a new $! to represent the triggering target.

Since my current need is only for $@ to consistency return a specific target, that is what I have implemented and want to share.   

In order to not force this behaviour change on multi-target pattern rules, I ended up adding a “grouped_target” file flag and only apply my change if it is set.  I didn’t want to add an extra flag, but other attempts at excluding pattern-rules quickly became messy.

Todd Lowe <toddlowe>

 

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

Attach Files:
   
   
Comment:
   

 

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 psmith (Posted a comment)
  • -email is unavailable- added by toddlowe (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-05-13 toddlowe Attached File- Added 0001-SV60077-add-grouped-targets-automatic-variables.patch, #51432
    2021-02-18 toddlowe Attached File- Added 0001-Make-always-return-the-1st-target-for-grouped-target.patch, #50880

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code