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.

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

Makefile.include: require make version 4. #10554

Merged
merged 1 commit into from Sep 23, 2019

Conversation

jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Dec 5, 2018

Contribution description

Old versions of GNU Make (especially on OSX) are a pain to deal with. This commit introduces a warning when such an old version is found. The goal is that in the future the warning will be turned into an error. For that I can open a PR, which we don't merge, and is tagged for a future release.

Complying with version 4.x is easy on OSX by using homebrew, and on Linux, even Debian stable has the required version.

Testing procedure

The test PR allows you to see what happens when you have an old version (it sets the version to 5, which does not exist at the moment.) Just go to any example and type make.

If you do the same without the test PR no warning is emitted.

Issues/PRs references

The old version of make in vanilla OSX is blocking the introduction of rules to make directories.
Old make does not support ?= in canned recipes.

@jcarrano jcarrano added Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system labels Dec 5, 2018
Makefile.include Outdated
@@ -1,3 +1,10 @@
MATCH_MAKE_VERSION = 5.%
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PLEASE NOTE The version here is 5 just to show what happens if you have an old version, so it can be tested.

Without the test commit, the required version is 4.

@kaspar030
Copy link
Contributor

Old versions of GNU Make (especially on OSX) are a pain to deal with.

Could you elaborate on that?

@jcarrano
Copy link
Contributor Author

jcarrano commented Dec 6, 2018

Ideally we would require the same version as in RIOTDOCKER. The reason is that we, build system maintainers, can easily test and develop on RIOTDOCKER, but not on OSX, so each time I have to do a fix on a makefile I have to work blind.

Important changes since make 3.81:

Version 3.82 (full list here)

  • The pattern-specific variables and pattern rules are now applied in the shortest stem first order instead of the definition order [...] This produces the usually-desired behavior where more specific patterns are preferred.
  • The 'define' make directive now allows a variable assignment operator after the variable name, to allow for simple, conditional, or appending multi-line variable assignment. For us, this will allow greater use of canned-recipes.

Version 4.0 (full list here)

  • New command line option: --output-sync (-O) enables grouping of output by target or by recursive make. This is useful during parallel builds to avoid mixing output from different jobs together giving hard-to-understand results.

Also, on one of this versions, a bug was fixed regarding rules ending in /, which is blocking us from fixing BINDIR creation and other stuff, like common build directories.

@jcarrano
Copy link
Contributor Author

jcarrano commented Dec 6, 2018

I'm sorry about the triplicate comments, github went bananas.

@jcarrano
Copy link
Contributor Author

What is the status with this? Mac users, is it tool burdensome to install make 4? (it is a sincere question, I've never used a mac in my life.)

@jcarrano jcarrano added the OS: Windows Host OS: This PR/issue concerns usage of RIOT with Windows as a host system label Mar 19, 2019
@jcarrano
Copy link
Contributor Author

What's up with this? Should we merge and then wait for the bug reports to come from Mac users? If I break the build that would surely get people interested.

@smlng
Copy link
Member

smlng commented May 28, 2019

using Home-brew you get gmake not make by default, to replace system make you'll need to add an export for PATH="/usr/local/opt/make/libexec/gnubin:$PATH" in ~/.profile - but that worked for me

@smlng
Copy link
Member

smlng commented May 28, 2019

What is the status with this? Mac users, is it tool burdensome to install make 4? (it is a sincere question, I've never used a mac in my life.)

using Home-Brew or MacPorts, which most developers do anyway, its straight forward and easy. But still we should have proper versions checks and warnings. for (all) tools we use and require. So this PR makes perfect sense to me for now - because ideally such dependency checks should be done in a system discovery (or bootstrap) step before the compile step; like ./configure ...

As we do not have that (yet), we should proceed with this PR.

@jcarrano
Copy link
Contributor Author

@smlng

to replace system make....

Is it necessary to replace system make?

But still we should have proper versions checks and warnings

Agreed, except checking for the make version is a special case because:

  • Make can self-check (i.e. no need to run external commands which is slow and the reason why we would want a separate step)
  • Make is the tool supposed to do the checking (or at least invoke it) so if the check for make is in a script which gets invoked by make it is already too late.

@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 28, 2019
@smlng
Copy link
Member

smlng commented May 29, 2019

@smlng

to replace system make....

Is it necessary to replace system make?

No, actually not - just prepend to PATH to find the newer version first

But still we should have proper versions checks and warnings

Agreed, except checking for the make version is a special case because:

  • Make can self-check (i.e. no need to run external commands which is slow and the reason why we would want a separate step)
  • Make is the tool supposed to do the checking (or at least invoke it) so if the check for make is in a script which gets invoked by make it is already too late.

yeah, what I meant was more like: not do the check over and over again but rather have something like ./configure to search for all tools and toolchains once on a system and store that permanently.

@jcarrano
Copy link
Contributor Author

No, actually not - just prepend to PATH to find the newer version first

Ok, I was asking because I have no idea about MacOS and your comment made me worry that the required setup was too invasive.

...but rather have something like ./configure to search....

yeah, yeah, I know. You cannot even imagine how often that topic comes up in conversations with @cladmi . Removing exports (WIP) should make the situation more tolerable, but it is not a full solution.

Anyways, I'm wandering off topic. Is everybody cool with this change?

@cladmi
Copy link
Contributor

cladmi commented Jun 3, 2019

I would like to update the phrasing on the error message. Could you put it as a deprecation warning somehow with a expiry date ? (== 2 releases)

As currently, we do not rely on stem target specific variables with multiple definitions, and no directories targets is currently in. We are thinking about the last one but could be done with */. with a documentation until then.

For the canned recipes, using instead the default-flash-recipe and flash-recipe ?= $(default-flash-recipe) is I think a better alternative as it gives a name that can be used to the default behavior.

I did not know --output-sync was only added in 3.8.2 though…

@jcarrano
Copy link
Contributor Author

jcarrano commented Jun 3, 2019

Could you put it as a deprecation warning somehow with a expiry date ? (== 2 releases)

Sure. It is also a good opportunity to start using deprecation warnings (and thus set a precedent).

@jcarrano
Copy link
Contributor Author

jcarrano commented Jun 3, 2019

I changed the message, see if you like it. In the meantime I disabled the CI to save a bit of CO2.

@jcarrano jcarrano requested review from fjmolinas and jia200x and removed request for cladmi, emmanuelsearch and kYc0o August 23, 2019 09:35
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

lets move this forward: (re)tested on MacOS as described before using GNU make installed via HomeBrew. ACK!

From my side: please squash and drop test commit.

Old versions of GNU Make (especially on OSX) are a pain to deal
with. This commit introduces a warning when such an old version
is found. The goal is the future the warning will be turned into
an error.

2020.01 is set as the date we remove support.

Complying with version 4.x is easy on OSX by using homebrew, and
on Linux, even Debian stable has the required version.
@jcarrano
Copy link
Contributor Author

@smlng done. sorry for the delay.

@smlng smlng merged commit 7bbdb74 into RIOT-OS:master Sep 23, 2019
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 29, 2019
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 OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system OS: Windows Host OS: This PR/issue concerns usage of RIOT with Windows as a host system Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants