Skip to content

Makefile.include: Fix call to sed for FreeBSD#12435

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
fhessel:fix-riotbuild-freebsd
Oct 28, 2019
Merged

Makefile.include: Fix call to sed for FreeBSD#12435
benpicco merged 1 commit intoRIOT-OS:masterfrom
fhessel:fix-riotbuild-freebsd

Conversation

@fhessel
Copy link
Copy Markdown
Contributor

@fhessel fhessel commented Oct 13, 2019

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

@benpicco benpicco 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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 13, 2019
@benpicco benpicco requested review from kaspar030 and smlng October 14, 2019 18:24
@benpicco benpicco mentioned this pull request Oct 28, 2019
Merged
@benpicco benpicco added this to the Release 2019.10 milestone Oct 28, 2019
benpicco
benpicco approved these changes Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Still builds fine on Linux, but fixes a build bug on FreeBSD and OS X.
Can you please provide a backport?
(Basically, submit the same PR again but with the 2019.10-branch branch as a target instead of the master branch, you should reference this PR.)

@benpicco benpicco added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 28, 2019
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 28, 2019

(you also can just use dist/tools/backport_pr/backport_pr.py ;-)

@benpicco benpicco requested a review from kYc0o October 28, 2019 10:03
@fhessel fhessel mentioned this pull request Oct 28, 2019
Merged
@fhessel
Copy link
Copy Markdown
Contributor Author

fhessel commented Oct 28, 2019

I opened #12591 and tested building the hello-world app on the 2019.10-brach on Debian and FreeBSD, which worked for both of them.

I opened the PR manually as I saw your comment too late, I hope it's still correct this way.

@benpicco benpicco merged commit c9166c6 into RIOT-OS:master Oct 28, 2019
@fhessel fhessel deleted the fix-riotbuild-freebsd branch October 28, 2019 10:29
@roberthartung
Copy link
Copy Markdown
Member

roberthartung commented Nov 20, 2019

@benpicco @miri64 This change breaks the build process for a colleague of mine. Which version of Mac OSX did you test on @fhessel ?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 20, 2019

@roberthartung which operating system is your colleague using?

@fhessel
Copy link
Copy Markdown
Contributor Author

fhessel commented Nov 20, 2019

As I said, I tested only on FreeBSD, because #12348 broke the build process there with the built-in version of sed. And as stated here, this doesn't fix the OSX build issue mentioned by @javierfileiv in #12183. However, he posted that after this PR was merged.

However, in contrast to the behavior on OSX mentioned in the referenced comment, on FreeBSD, riotbuild.h was created and populated with the same content that it contains on a Linux host. So this may be two different problems.

@janschlicht
Copy link
Copy Markdown

janschlicht commented Nov 20, 2019

@miri64 I'm using MacOs 10.15.1 and it fails with
sed: 1: "1i/* Generated file do ...": command i expects \ followed by text
The line break seems to be the problem if I move the call to one line it works fine.

@fhessel
Copy link
Copy Markdown
Contributor Author

fhessel commented Nov 20, 2019

The line break seems to be the problem if I move the call to one line it works fine.

From that, I'd assume that ...

  • GNU sed on Linux tolerates both syntax variants
  • Built-in sed on FreeBSD requires the one with a line break
  • sed on the MAC requires the one without a line break.

… which would make it hard to solve independent of the platform. Maybe the following alternative from the PR description would have been the better solution, as it does not require an i command in sed?

Something like echo '/* ... */' | cat - $< | sed ... > $@ could be an alternative.

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

Reviewers

@benpicco benpicco benpicco approved these changes

@smlng smlng Awaiting requested review from smlng

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@kYc0o kYc0o Awaiting requested review from kYc0o

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: needs backport Integration Process: The PR is required to be backported to a release or feature branch 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.

5 participants

@fhessel @miri64 @roberthartung @janschlicht @benpicco