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

Fix make and makefile #169

Merged
merged 7 commits into from
Oct 26, 2020
Merged

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Oct 24, 2020

CLI has the same problem as Engine with share/Makefile not being compatible with GNU Make on CentOS 7 (zonemaster/zonemaster-engine#813). The solution, as implemented by zonemaster/zonemaster-engine#814, made share/Makefile incompatible with BSD Make (on FreeBSD).

A partial solution to the problem for FreeBSD is proposed in zonemaster/zonemaster-engine#815.

This PR proposes a complete solution to the problem. GNU Make and BSD Make cannot sucessfully use the same Makefile, but GNU Make can be made available on FreeBSD as gmake. And for GNU Make the file GNUmakefile is the preferred file, if available. We use that fact.

  • Update Makefile to be GNU Make compatible for CentOS 7 (and incompatible for BSD Make).
  • Rename Makefile to GNUmakefile.
  • Create wrapper Makefile for BSD Make to make it run the same target as gmake (and GNUmakefile).
  • Update Makefile.PL to require gmake for FreeBSD and to use gmake for share/GNUmakefile under FreeBSD.
  • Update installation instructions to include the installation of gmake.

The separation into two makefiles will make it possible to update those for future use.

@matsduf matsduf added T-Bug Type: Bug in software or error in test case description P-High Priority: Issue to be solved before other A-BuildSystem labels Oct 24, 2020
@matsduf matsduf added this to the v2020.1 milestone Oct 24, 2020
@matsduf matsduf requested review from mattias-p, sandoche2k and a user October 24, 2020 20:49
@matsduf
Copy link
Contributor Author

matsduf commented Oct 24, 2020

@sandoche2k, before you test build CLI on CentOS 7, review and test this PR.

Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

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

This is a quite non-trivial change. I don't think we should be merging this kind of thing this close to the release. It's better to adopt the same solution we used in Engine. We can do loftier things for the next release.

Makefile.PL Outdated Show resolved Hide resolved
Makefile.PL Outdated
Comment on lines 34 to 44
my $text;
if ($^O eq "freebsd") {
# Make FreeBSD use gmake for share
$text = q[GMAKE ?= "gmake"] . "\n"
. q[pure_all :: ] . "$sharemakefile\n"
. "\t" . q[cd share && $(GMAKE) all] . "\n";
} else {
# Here Linux and GNU Make is assumed
$text = q[pure_all :: ] . "$sharemakefile\n"
. "\t" . q[cd share && $(MAKE) all] . "\n";
};
Copy link
Member

Choose a reason for hiding this comment

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

Why not make both cases use the same variable for the make command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The $(MAKE) variable comes from the Makefile. The variable is interpolated by make.

Copy link
Member

Choose a reason for hiding this comment

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

I think I would have gone for setting GMAKE to different values on different platforms. That way the "pure_all" clause could be the same for all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such as "GMAKE = $(MAKE)" and "GMAKE = gmake", respectively?

Well, it has the same effect. Please the updated code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's what I had in mind. I'm thinking that it's a little easier on the reader (i.e. us) if you don't have to compare two heavily quoted and generated makefile snippets by eye.

@matsduf
Copy link
Contributor Author

matsduf commented Oct 26, 2020

@mattias-p and @sandoche2k, please review again.

@mattias-p
Copy link
Member

@matsduf I can't point to any technical errors in this PR, but do you really think we should make changes of this complexity this close to the release? We have much greater chances of finding problems with the Makefile-GNUmakefile approach if we merge it in the beginning of a release cycle. I think we should go for a more modest solution for this release.

@matsduf
Copy link
Contributor Author

matsduf commented Oct 26, 2020

We have much greater chances of finding problems with the Makefile-GNUmakefile approach if we merge it in the beginning of a release cycle.

It is well-documented that GNU Make prefers GNUmakefile before Makefile. The only problem would be the wrapper function, but that only affects FreeBSD which does not work well with the current situation anyway. I do not see that the risks are higher to implement than to keep it.

I am surprised that we have not seen the solution before instead of trying to make Makefile work for both GNU Make and BSD Make.

@mattias-p
Copy link
Member

It's true that GNU make will pick the GNUmakefile.

But it's no beauty queen. I guess that could be a reason why we haven't seen it before. For the next release I'll see if I can't come up with something more satisfying.

@matsduf
Copy link
Contributor Author

matsduf commented Oct 26, 2020

Unless the makefile is very simple it does not seem to be possible to share it between BSD Make and GNU Make. I think this solution of having a wrapper for BSD Make is quite sound. Else we could leave make and use a shell script instead.

@matsduf matsduf merged commit 1b2dfdd into zonemaster:develop Oct 26, 2020
@matsduf matsduf deleted the fix-make-and-makefile branch October 26, 2020 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-High Priority: Issue to be solved before other T-Bug Type: Bug in software or error in test case description
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants