Sort sample Makefile to be more inline with chapter guidelines
ClosedPublic

Authored by ultima on Jul 7 2017, 7:14 PM.

Details

Summary

The sample Makefile is a bit out of order. This patch will sort the sample Makefile to be more inline with the following sections of the chapter.

Reviewed by\: lifanov (mentor), matthew (mentor), docguru
Approved by\: lifanov (mentor), matthew (mentor), docguru
Differential Revision\: https://reviews.freebsd.org/DXXXXX

One other thing I would like to point out. The USES and USE_x
chapter doesn't mention anything about variables like
GNU_CONFIGURE and WRKSRC. It maybe a good idea to make it more
clear that this section block is for configuring not just
strictly for USES and USE_x. Another alternative is making a
new section block under the USES and USE_x for the other
configuration variables. The former is probably best.

Diff Detail

Repository
rD FreeBSD doc repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ultima created this revision.Jul 7 2017, 7:14 PM
ultima updated this revision to Diff 30548.Jul 7 2017, 7:23 PM

Combind the two USES as the Makefile should never have two USES variables.

It's mat who does most of the work on the Porter's Handbook.

en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
73 ↗(On Diff #30548)

This has a different effect than the original code, since in:

USES= gmake
USES= imake

the 2nd line cancels out the first... However, I think your version is more like
what was intended.

ultima updated this revision to Diff 30557.Jul 7 2017, 10:48 PM
  • Add USE_GITHUB and GH_* Under USES
  • Added OPTION section to the sample Makefile
ultima added inline comments.Jul 7 2017, 10:50 PM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
80 ↗(On Diff #30557)

Ah, sorry whitespace change isn't relevant

ultima updated this revision to Diff 30558.Jul 7 2017, 10:51 PM

Removed whitespace change

mat added inline comments.Jul 11 2017, 11:34 AM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
73 ↗(On Diff #30548)

No, this, here, is not supposed to be a working Makefile, it is supposed to show what you might want. Using both at the same time, here, is a bad idea. It kinda tells you that you are going to need both, whereas there are only 6 ports using both, out of the 270+ using imake and 4k using gmake.

So I would rather this be kept as it was, two separate bits.

59 ↗(On Diff #30558)

while you are changing this file, s/we no longer accept/we do not accept/

89 ↗(On Diff #30558)

maybe add an OPTIONS_SUB=yes here.

mat added a comment.Jul 11 2017, 11:35 AM

GNU_CONFIGURE implies USE_CONFIGURE, so it can be kept with the USE_* block. Maybe I'll change GNU_CONFIGURE to USE_CONFIGURE=gnu.

ultima updated this revision to Diff 30737.Jul 13 2017, 6:41 PM
  • Changed line 59 as suggested
  • Reverted the gmake/imake merge
  • Added OPTION_SUB with small comment
ultima marked 4 inline comments as done.Jul 13 2017, 6:42 PM
ultima marked an inline comment as done.
ultima updated this revision to Diff 30738.Jul 13 2017, 7:04 PM
  • Added new section, Standard bsd.port.mk Variables
mat added inline comments.Jul 14 2017, 8:20 AM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
75 ↗(On Diff #30738)

should probably add a comment above this to separate it from imake.

91 ↗(On Diff #30738)

s/add/change/ or something less terse.

493–495 ↗(On Diff #30738)

You should probably add some examples here, this is too vague.

wblock added inline comments.Jul 14 2017, 3:50 PM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
73 ↗(On Diff #30548)

Maybe this is not a good example any more. If multiple options are to be shown, better to show a couple of complete, working sample Makefiles rather than one that does not work as shown.

52 ↗(On Diff #30738)

Let's make these real DocBook callouts so they are not mixed with the actual Makefile contents.
See https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/docbook-markup-block-elements.html#docbook-markup-callouts

493 ↗(On Diff #30738)

s/are definable/can be defined/

494 ↗(On Diff #30738)

Remove "the", and "belong to" is kind of confusing. "belong in" is a little better, but how is the reader supposed to know where things belong?

ultima updated this revision to Diff 30795.Jul 14 2017, 8:07 PM
ultima marked 4 inline comments as done.

Added more details to new section, added callouts but will probably require revision.

ultima marked an inline comment as done.Jul 14 2017, 8:10 PM
ultima added inline comments.
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
52 ↗(On Diff #30738)

Not sure I completely understand how the callouts work, can you check if this looks correct? If so, should this be done for the rest of the sample Makefile?

493–495 ↗(On Diff #30738)

I think this section is looking much better now!

120 ↗(On Diff #30795)

I think the callout list is supposed to be under the programlisting, but I'm not sure what this block should be, or is this correct?

mat added inline comments.Jul 15 2017, 9:31 AM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
73 ↗(On Diff #30548)

It never was a good example, it tells you what kind of stuff you can put in the Makefile, and where, but it never was a working example of what to do exactly.
I am not sure it makes sense to have multiple examples either, they would probably get outdated fast.

ultima marked an inline comment as done.Jul 15 2017, 4:54 PM
ultima added inline comments.
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
73 ↗(On Diff #30548)

If this samples [ ]comments are all converted to callouts, combining the USES and adding a call out next to them will make it easily understood that they they are for a specific purpose. I think the callout suggestion is a good idea, I am worried though that in the end there will be so many callouts that this sample won't look so great.

mat added a comment.Jul 18 2017, 12:41 PM

(please, pkg install docproj and run make in that directory to make sure it builds)

en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
52 ↗(On Diff #30795)

The ref is co-patch-dist-strip.

117 ↗(On Diff #30795)

Either <literal>${WRKSRC}</literal> or <filename>${WRKSRC}</filename>

Also, at the end, it should be </para>

120 ↗(On Diff #30795)

It won't build with the calloutlist inside the programlisting.

497 ↗(On Diff #30795)

dupplicated id.

mat added inline comments.Jul 19 2017, 2:05 PM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
52 ↗(On Diff #30738)

Well, if all the [...] go into callouts, the page will be unreadable, there will be 20+ callouts, and people will keep scrolling up and down to figure out what means what.

ultima updated this revision to Diff 31017.EditedJul 20 2017, 5:56 PM

Removed Removed calloutlist bits. While I like this idea, I would rather a docs committer make these changes. I also feel that the page maybe harder to read as a result, going to leave this out of the commit.

I also created a patch and added it to misc/freebsd-doc-en to build for QA. I'm not familiar with proper QA for docs, but after creating the patch and adding it in files/patch-file.diff, it builds correctly.

mat added inline comments.Jul 25 2017, 12:54 PM
en_US.ISO8859-1/books/porters-handbook/porting-samplem/chapter.xml
500–501 ↗(On Diff #31017)

Move the replaceable bits inside varname.

502 ↗(On Diff #31017)

Please remove the evil PORTDATA, we want to remove it.

ultima updated this revision to Diff 31176.Jul 25 2017, 1:29 PM
ultima marked an inline comment as done.
  • Corrected varname open/close
  • Slayed a spawn of Evil
mat accepted this revision as: mat.Jul 25 2017, 2:35 PM

Looks ok to me.

Do make sure it builds. (pkg install docproj, and run make in the porters-handbook directory)

matthew accepted this revision.Jul 25 2017, 2:51 PM

Approved, subject to running the build test mat specified.

The build Doesn't have any errors, a single warning, but it is irrelevant to this patch, built in poudriere.

Warning: Object directory not changed from original /root/doc/en_US.ISO8859-1/books/porters-handbook

This revision was automatically updated to reflect the committed changes.