10000 specify the minimum version of make by silenceshell · Pull Request #389 · weaveworks/ignite · GitHub
[go: up one dir, main page]

Skip to content
This repository was archived by the owner on Dec 7, 2023. It is now read-only.

specify the minimum version of make #389

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

silenceshell
Copy link
Contributor

fix #383

undefine (line 20) exists in GNU make 3.82 and higher, however on some os make is still 3.81 or even lower, for example macOS 10.14.

We should check make version in Makefile.

@silenceshell silenceshell requested a review from twelho as a code owner September 3, 2019 10:03
Copy link
Contributor
@stealthybox stealthybox left a comment

Choose a reason for hiding this comment

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

Thanks for this -- I've tested it on my linux machine and the version gate works.

Would you please parameterize it like this? Cheers

MIN_MAKE_VERSION = 3.82
ifneq ($(MIN_MAKE_VERSION), $(firstword $(sort $(MAKE_VERSION) $(MIN_MAKE_VERSION))))
$(error this project requires make version $(MIN_MAKE_VERSION) or higher)
endif

Makefile Outdated
@@ -1,3 +1,7 @@
ifneq (3.82, $(firstword $(sort $(MAKE_VERSION) 3.82)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just noting here that this is a lexical sort (ref)

This would eventually break with Make version 10, which is not a near-term concern.

Copy link
Member

Choose a reason for hiding this comment

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

@silenceshell could you try to compute with bc or something like this:

https://stackoverflow.com/a/21491594

@silenceshell
Copy link
Contributor Author

@stealthybox @chanwit
Thank you for your response. I have updated the code, please help to review :)

@chanwit
Copy link
Member
chanwit commented Sep 4, 2019

LGTM

@stealthybox
Copy link
Contributor

Thanks for the love on this patch @silenceshell

@stealthybox stealthybox merged commit 8bd9ae2 into weaveworks:master Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not able to run make build-all
3 participants
0