Skip to content

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jan 23, 2019

Contribution description

It is only used by Makefile.include and should not be used by sub-make
instances.

This is required to prevent evaluating LINKFLAGS when not needed and a
required step to not evaluate it on the host with for example avr-ld
when building in docker.

I checked usage with:

git grep -e '(LINKFLAGS)' -e '{LINKFLAGS}'

Packages may be using it but LINKFLAGS is a RIOT way of naming
things and even if generate-xcompile-toolchain uses LINK it does not
use LINKFLAGS.

Testing procedure

Look at the usages on LINKFLAGS with git grep -e '(LINKFLAGS)' -e '{LINKFLAGS}' and notice nothing is using it except Makefile.include and makefiles/info.inc.mk.

I did not look in the packages yet but will if the CI build fails.

Issues/PRs references

Found while testing arduino-mega2560 in docker on a machine without avr installed. /bin/sh: 1: avr-ld: not found. It needs another commit to be fixed though that will come in another PR.

Main reference on removing exports: #10850

It is only used by `Makefile.include` and should not be used by sub-make
instances.

This is required to prevent evaluating `LINKFLAGS` when not needed and a
required step to not evaluate it on the host with for example `avr-ld`
when building in docker.

I checked usage with:

    git grep -e '(LINKFLAGS)' -e '{LINKFLAGS}'

Packages may be using it but `LINKFLAGS` is a RIOT way of naming
things and even if `generate-xcompile-toolchain` uses `LINK` it does not
use `LINKFLAGS`.
@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 labels Jan 23, 2019
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

The provided reason for this change and the change itself make both sense.

I also tested #10854 and also confirm that this PR is fixing the bug (I already noticed it some time ago but didn't understand why it was failing).

The build CI is green, so the change is not introducing any problems.

ACK

@aabadie aabadie merged commit 3a1a615 into RIOT-OS:master Jan 23, 2019
@aabadie aabadie added this to the Release 2019.01 milestone Jan 24, 2019
@cladmi cladmi deleted the pr/make/unexport/linkflags branch January 25, 2019 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants