Skip to content

Makefile.include: Fix call to sed for FreeBSD and OS X [backport 2019.10]#12591

Merged
miri64 merged 1 commit intoRIOT-OS:2019.10-branchfrom
fhessel:backports/fix-riotbuild-freebsd
Oct 28, 2019
Merged

Makefile.include: Fix call to sed for FreeBSD and OS X [backport 2019.10]#12591
miri64 merged 1 commit intoRIOT-OS:2019.10-branchfrom
fhessel:backports/fix-riotbuild-freebsd

Conversation

@fhessel
Copy link
Copy Markdown
Contributor

@fhessel fhessel commented Oct 28, 2019

Backport of #12435

Contribution description

Disclaimer: I'm not an expert on FreeBSD, but only stumbled upon this while using it to test another PR on a non-Linux platform and wanted to let you know.

If I didn't miss something, it looks like merging PR #12348 has broken the build on FreeBSD, as it uses sed's insert functionality with a syntax that is tolerated by GNU's sed but not by the one on FreeBSD. This PR changes the call to sed in a way that works on both platforms.

The call in question is:

$(Q)sed -n -e '1i /* Generated file do not edit */' -e '/^#.*/ p' $< > $@

From sed's manpage, we see that a backslash followed by a new line is required for the i command:

Zero- or One- address commands
[...]
  i \
  text   Insert text, which has each embedded newline preceded by a backslash.

While GNU's sed under Linux accepts the syntax used in PR #12348, the build with FreeBSD fails (master was on cce1438 for my tests, I also tried 3bfe30a in isolation):

$ export CC=gcc47
$ export LINK=gcc47
$ git checkout master
$ BOARD=native gmake -C examples/hello-world clean all
gmake: Entering directory '/usr/home/frank/riot/RIOT/examples/hello-world'
Building application "hello-world" for "native" with MCU "native".
sed: 1: "1i /* Generated file do ...": command i expects \ followed by text
gmake: *** [/usr/home/frank/riot/RIOT/examples/hello-world/../../Makefile.include:821: /usr/home/frank/riot/RIOT/examples/hello-world/bin/native/riotbuild/riotbuild.h] Error 1
gmake: Leaving directory '/usr/home/frank/riot/RIOT/examples/hello-world'

Using commit 3f0dfc1 (immediately before the PR was merged) was still successful on FreeBSD:

$ export CC=gcc47
$ export LINK=gcc47
$ git checkout 3f0dfc14ac2f1dc7cc1546abc5307fb93f866b26 # Commit just before the PR was merged
$ BOARD=native gmake -C examples/hello-world clean all
gmake: Entering directory '/usr/home/frank/riot/RIOT/examples/hello-world'
Building application "hello-world" for "native" with MCU "native".
"gmake" -C /usr/home/frank/riot/RIOT/boards/native
"gmake" -C /usr/home/frank/riot/RIOT/boards/native/drivers
"gmake" -C /usr/home/frank/riot/RIOT/core
"gmake" -C /usr/home/frank/riot/RIOT/cpu/native
"gmake" -C /usr/home/frank/riot/RIOT/cpu/native/periph
"gmake" -C /usr/home/frank/riot/RIOT/cpu/native/vfs
"gmake" -C /usr/home/frank/riot/RIOT/drivers
"gmake" -C /usr/home/frank/riot/RIOT/drivers/periph_common
"gmake" -C /usr/home/frank/riot/RIOT/sys
"gmake" -C /usr/home/frank/riot/RIOT/sys/auto_init
   text   data      bss      dec       hex   filename
  21205    368   128136   149709   0x248cd   /usr/home/frank/riot/RIOT/examples/hello-world/bin/native/hello-world.elf
gmake: Leaving directory '/usr/home/frank/riot/RIOT/examples/hello-world'

The formatting of the fix is not very nice, but using an actual line break was the only solution that allowed staying with sed only and worked for me independently of the host platform and shell. Something like echo '/* ... */' | cat - $< | sed ... > $@ could be an alternative.

Testing procedure

To verify that this issue exists, checkout the current master on FreeBSD, try to build an application (e.g. examples/hello_world), and see that the aforementioned error occurs:

export CC=gcc47
export LINK=gcc47
git checkout master
BOARD=native gmake -C examples/hello-world clean all

After applying this change, the error should be gone. Also verify that the content of <appdir>/bin/native/riotbuild/riotbuild.h has not changed with and without the PR.

Issues/PRs references

See #12348

@fhessel fhessel mentioned this pull request Oct 28, 2019
Merged
@miri64 miri64 added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 28, 2019
@miri64 miri64 added this to the Release 2019.10 milestone Oct 28, 2019
@benpicco benpicco changed the title Makefile.include: Fix call to sed for FreeBSD [backport 2019.10] Makefile.include: Fix call to sed for FreeBSD and OS X [backport 2019.10] Oct 28, 2019
miri64
miri64 approved these changes Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Change is identical to #12435 and was tested by contributor for FreeBSD. ACK.

@benpicco benpicco requested a review from basilfx October 28, 2019 10:28
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 28, 2019

Also tested on OS X.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 28, 2019

Also tested on OS X.

I don't understand the link. It is leading to a PR only slightly related and the issue comment doesn't seem to be in that PR

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 28, 2019

@javierfileiv deleted the comment I was linking to - looks like he has trouble reproducing it after all :\

It came up in this PR that he reported the build would fail on OS X due to the sed call. I suggested he should try this patch.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 28, 2019

The PR to master was tested, so no sense stalling this. If more issues are found, they need to be fixed in master first anyways

@miri64 miri64 merged commit b412b7f into RIOT-OS:2019.10-branch Oct 28, 2019
@javierfileiv
Copy link
Copy Markdown
Contributor

javierfileiv commented Oct 28, 2019

Wasn't able to make it work using MAC-OS. As I said on #12183 (comment)

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 28, 2019

Thank @benpicco ... it didn't work... actually riotbuild.h.in gets generated but riotbuild.h it's empty. What I have done before knowing @vincent-d patch was to only create riotbuild.h, without using the .in file. But as I said before, with the steps that Vincent told me was working flawlessly

Well ideally you should be able to build RIOT without any extra steps - do you see the issue on master too or just with the #12183 PR?

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

Reviewers

@miri64 miri64 miri64 approved these changes

@basilfx basilfx Awaiting requested review from basilfx

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

4 participants

@fhessel @benpicco @miri64 @javierfileiv