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.

Sign up for GitHub

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

Jump to bottom

Makefile: fix install failure if path contains "m4/" string #49

Merged
merged 1 commit into from Jan 19, 2020
Merged

Makefile: fix install failure if path contains "m4/" string #49

merged 1 commit into from Jan 19, 2020

Conversation

giuliobenetti
Copy link
Contributor

Actually Makefile install recipe substitutes every occurence of "m4/" in
file name of the target of the rule($@), in an absolute path there could
more than one "m4/" occurence, so install will fail. Let's change
$(susbst ...) call with a call to sed substituting only last occurence
of "m4/".

Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com

@giuliobenetti giuliobenetti mentioned this pull request Jan 17, 2020
Closed
yann-morin-1998
yann-morin-1998 reviewed Jan 17, 2020
View reviewed changes
Makefile Outdated
@@ -88,6 +88,6 @@ $(DESTDIR)$(datadir)/%: %
$(INSTALL) -D -m 644 $< $@

$(DESTDIR)$(acdir)/%: %
$(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(subst m4/,,$@)
$(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(shell echo $@ | sed 's,\(.*\)m4/,\1,')
Copy link

Choose a reason for hiding this comment

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

What about:

- $(subst m4/,,$@)
+ $(patsubst %m4/,%,$@)

Copy link

@hthiery hthiery Jan 17, 2020 •
edited

Choose a reason for hiding this comment

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

Indeed. This also works and is more elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is more elegant. Tomorrow I'll resend the patch here and to Buildroot ML.
Thank you for proposing and testing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehm this doesn't work for me instead. @hthiery can you please give it a try and check if it works for you? Basically it doesn't do the trick to remove "m4/", so I find all .m4 files in:
host/share/aclocal/m4/
instead of:
host/share/aclocal/

Copy link

Choose a reason for hiding this comment

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

you're right ... this does not work

@giuliobenetti
Copy link
Contributor Author

giuliobenetti commented Jan 17, 2020 •
edited

Sorry but this patch doesn't work for me, see:
#49 (comment)

@xhebox
Copy link
Collaborator

xhebox commented Jan 18, 2020 •
edited

OK. I notice a much simpler patch:

--- a/Makefile
+++ b/Makefile
@@ -88,6 +88,6 @@ $(DESTDIR)$(datadir)/%: %
        $(INSTALL) -D -m 644 $< $@
 
 $(DESTDIR)$(acdir)/%: %
-       $(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(subst m4/,,$@)
+       $(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(DESTDIR)$(acdir)/$(subst m4/,,$<)
 
 .PHONY: all clean install

EDIT: but i dont quite understand why patsubst does not work.

@hthiery
Copy link

hthiery commented Jan 18, 2020 •
edited

EDIT: but i dont quite understand why patsubst does not work.
I think the reason why the patsubst approch does not work is because the pattern does not match.

$@ contains the full path to the desired target including the name of the file. But the rule only matches if it ends with m4/.

--- a/Makefile
+++ b/Makefile
@@ -88,6 +88,6 @@ $(DESTDIR)$(datadir)/%: %
        $(INSTALL) -D -m 644 $< $@
 
 $(DESTDIR)$(acdir)/%: %
-       $(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(subst m4/,,$@)
+       $(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(patsubst %m4,%,$(@D))/$(@F)
 
 .PHONY: all clean install

If I'm correct this should work. This replaces the m4 from the path $(@D) and adds the filename $(@F).

Note: $(@D) does not contain the trailing /.

@xhebox
Copy link
Collaborator

xhebox commented Jan 18, 2020

If I'm correct this should work.

Confirmed. But i noticed this in gnu doc:

These variants are semi-obsolete in GNU make since the functions dir and notdir can be used to get a similar effect (see Functions for File Names).

So i would suggest use function instead of magic $(@D):

$(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(patsubst %m4/,%,$(dir $@))/$(notdir $@)

@hthiery
Copy link

hthiery commented Jan 18, 2020

If I'm correct this should work.

Confirmed. But i noticed this in gnu doc:

These variants are semi-obsolete in GNU make since the functions dir and notdir can be used to get a similar effect (see Functions for File Names).

So i would suggest use function instead of magic $(@D):

ok ... i was not aware of these functions...

thank you

$(INSTALL) -D -l ../$(subst $(datarootdir)/,,$(datadir))/$< $(patsubst %m4/,%,$(dir $@))/$(notdir $@)

@giuliobenetti
Copy link
Contributor Author

@hthiery do you want to take over and submit this patch? Basically you've found the cleanest way. Otherwise I will do it.

@hthiery
Copy link

hthiery commented Jan 18, 2020

@hthiery do you want to take over and submit this patch? Basically you've found the cleanest way. Otherwise I will do it.

You can do that. No problem. I just wanted to help.

@giuliobenetti
Makefile: fix install failure if path contains "m4/" string
2ad9d66 
Actually Makefile install recipe substitutes every occurence of "m4/" in
file name of the target of the rule($@), in an absolute path there could
more than one "m4/" occurence, so install will fail. Let's change
$(subst ...) with $(patsubst ...) substituting only last occurence of
"m4/" pattern.

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
@giuliobenetti
Copy link
Contributor Author

Here is the corrected patch.

King regards

@xhebox xhebox merged commit adaa9c6 into sabotage-linux:master Jan 19, 2020
@hthiery
Copy link

hthiery commented Jan 19, 2020

Here is the corrected patch.

King regards

@giuliobenetti thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yann-morin-1998 yann-morin-1998 yann-morin-1998 left review comments

@hthiery hthiery hthiery left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@giuliobenetti @xhebox @hthiery @yann-morin-1998