Skip to content
This repository was archived by the owner on Mar 5, 2024. It is now read-only.

Fix builds on BSD platforms by invoking "gmake", not "make". #989

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

ke6jjj
Copy link
Contributor

@ke6jjj ke6jjj commented Sep 11, 2021

No description provided.

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 14, 2021

I don't like that my patch leaves no "fall-through" default when the operating system isn't one of the ones explicitly listed in the build instructions. But I'm simply using the pattern I saw used with the rebar.config in sibyl. If anyone has a good method for adding the fallthrough, that would be awesome!

@Vagabond
Copy link
Contributor

One trick I've done in the past is that GNU make will look for a GNUMakefile before it looks for a Makefile, so you can put all your real make rules in the GNUMakefile and have a Makefile (that BSD make will look for first) that just calls gmake with the target it was provided.

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 14, 2021

I like that idea, and I swear I even suggested this in the past, but I seem to recall that we found that solution even more abhorrent for some reason. I think that conversation happened between me at @madninja over a year ago.

@Vagabond
Copy link
Contributor

I like it because it resolves which version of make you have as make regardless of platform (some platforms don't include a native make so it can depend on your package manager, etc).

@madninja
Copy link
Member

I like it because it resolves which version of make you have as make regardless of platform (some platforms don't include a native make so it can depend on your package manager, etc).

but wouldn't bsd make calling make then not require gmake to be installed on all other platforms?

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

but wouldn't bsd make calling make then not require gmake to be installed on all other platforms?

You'd still need gnumake everywhere: the main Makefiles are written in its syntax. This method just bootstraps BSD make to make it run gnumake.

@madninja
Copy link
Member

madninja commented Sep 15, 2021

but wouldn't bsd make calling make then not require gmake to be installed on all other platforms?

You'd still need gnumake everywhere: the main Makefiles are written in its syntax. This method just bootstraps BSD make to make it run gnumake.

Really? I build miner on my Mac laptop and

gmake
zsh: command not found: gmake

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

Really? I build miner on my Mac laptop and

gmake
zsh: command not found: gmake

The native make on Mac OS X is in fact gnumake. I know we often think of Mac OS X as "BSD", but it's not in this particular regard.

@madninja
Copy link
Member

Really? I build miner on my Mac laptop and

gmake
zsh: command not found: gmake

The native make on Mac OS X is in fact gnumake. I know we often think of Mac OS X as "BSD", but it's not in this particular regard.

Right, so blindly calling gmake isn't going to work right? Or is that not what you're suggesting?

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

Right, so blindly calling gmake isn't going to work right? Or is that not what you're suggesting?

Not what was suggested. The suggestion is:

  1. Rename the master Makefile to GNUMakefile
  2. Create a new Makefile whose rules simply invoke gmake GNUMakefile (or just gmake to be honest).

Anyone whose native make is BSD make will pick up the new Makefile, and thus invoke gmake GNUMakefile
Anyone whose native make is gnumake will pick up the GNUMakefile and proceed normally.

Thus, all the special rebar.config rules can be removed and everyone can simply invoke make on their platform and expect the correct result.

@madninja
Copy link
Member

Right, so blindly calling gmake isn't going to work right? Or is that not what you're suggesting?

Not what was suggested. The suggestion is:

  1. Rename the master Makefile to GNUMakefile
  2. Create a new Makefile whose rules simply invoke gmake GNUMakefile (or just gmake to be honest).

Anyone whose native make is BSD make will pick up the new Makefile, and thus invoke gmake GNUMakefile
Anyone whose native make is gnumake will pick up the GNUMakefile and proceed normally.

Gotcha, so this is saying the BSD users will have to have gmake installed?

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

Yes, anyone who is on BSD will have gmake (not at first, but anyone building just about anything will know this).

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

@Vagabond @madninja It sounded great..Until I just tried it. GNUMake doesn't appear to favor GNUMakefile over Makefile. If Makefile exists, it uses it first. 😢 That's probably what I discovered long ago and what prevented me from submitting this as the fix.

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

GNUMakefile
-----
all:
    echo GNUMakefile used
-----
Makefile
-----
all:
    echo Makefile used

Then, on OS X:

$ make
echo Makefile used
Makefile used
$

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

Sorry for the spam. Guess what? It actually works!

The GNUMakefile has to be correctly cased: GNUmakefile (little m)

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

Here's a proposed Makefile which should invoke gmake with the correct target, no matter what was provided on the command line:

all:
        gmake

$(.TARGETS):
        gmake $(.TARGETS)

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 15, 2021

Summary

We have some options. This GNUmakefile is kind of ugly, but it works. Whether that will continue to be understood by future and new developers is a toss up. The other option is to keep the complexity in rebar.config, which, at the very least, is a bit easier to understand: "Oh, on some platforms it invokes make and on others it invokes gmake".

I'm ok with either one. I don't want to rock the boat too hard.

@Vagabond
Copy link
Contributor

I dislike the platform approach because it requires us to maintain a list of platform to make type mappings whereas the Makefile based approach works no matter what system you are on or what you have make set as (on some platforms this is not defined and can change depending on user preference).

I think as long as we have a big comment in the Makefile saying "This is a stub, go edit GNUmakefile instead" it will be fine and it will essentially be 0 maintenance from there on out.

@Vagabond
Copy link
Contributor

You could also, in the BSD Makefile, tell the user they need to install GNU make if gmake is not in the PATH. You can't do that via the rebar approach.

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 21, 2021

This sounds good to me! I will squash these commits once again and enact what you've suggested, @Vagabond .

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 21, 2021

N.B. There are a few other rebar projects that should take this new approach. E.g. sibyl.

@ke6jjj ke6jjj force-pushed the jsc/fix-implicit-make-is-gnumake-1 branch 2 times, most recently from b3b09d5 to f04a6f5 Compare September 21, 2021 18:41
@Vagabond
Copy link
Contributor

I like this, does @madninja have any objections? (and we should indeed do this on other projects).

@madninja
Copy link
Member

I like this, does @madninja have any objections? (and we should indeed do this on other projects).

I like it 👍🏻

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 22, 2021

:shipit: (🚢 It)

@Vagabond
Copy link
Contributor

oh, did you use git mv to rename Makefile to GNUmakefile or did you copy the file? it looks like you did the latter which is going to destroy commit history on the file (which is a bit annoying).

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 22, 2021

I used git mv, but since everyone is adamant that every commit on a PR be squashed together, I did that too. Maybe that's what killed it?

(I'm not a fan of the squash).

@ke6jjj ke6jjj force-pushed the jsc/fix-implicit-make-is-gnumake-1 branch from f04a6f5 to 8dea4fc Compare September 22, 2021 17:43
@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 22, 2021

I've unsquashed and re-forced the commit. It should appear as a mv now.

@ke6jjj ke6jjj force-pushed the jsc/fix-implicit-make-is-gnumake-1 branch from 8dea4fc to 113d659 Compare September 22, 2021 17:47
Part 1: Rename Makefile -> GNUmakefile

Adopt a solution to the problem where the system "make" utility is not
a GNU-compatible variant by taking advantage of the way GNU Make searches
for its Makefile differently from traditional (AT&T V7) make and redirecting
the build process to use "gmake", which is the conventional name for GNU Make
on such platforms.
@ke6jjj ke6jjj force-pushed the jsc/fix-implicit-make-is-gnumake-1 branch from 113d659 to c3e6836 Compare September 22, 2021 17:52
@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 22, 2021

It really looks to be a GitHub bug. I have a Gitea instance I run at home and it preserves the history with this latest commit. GitHub gets confused.

@Vagabond
Copy link
Contributor

good enough. We will merge this shortly, thanks.

@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 22, 2021

aaaand, I've rechecked and GitHub seems ok now. I think the issue is that this needs to stay as two separate commits; we can't squash into one or we trigger the bug. Can you confirm, @Vagabond that the history now looks to be preserved in GNUmakefile?

@ke6jjj ke6jjj force-pushed the jsc/fix-implicit-make-is-gnumake-1 branch from c3e6836 to 1b95585 Compare September 22, 2021 18:03
@ke6jjj
Copy link
Contributor Author

ke6jjj commented Sep 22, 2021

(I am truly sorry for the spam; I just updated the commit message in the second commit, thus the latest force push)

@Vagabond
Copy link
Contributor

Shall we merge @madninja ?

@madninja madninja merged commit 2ea996e into helium:master Sep 27, 2021
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.

3 participants