Details
- Reviewers
ygy woodsb02 yuri - Commits
- rP474984: Add new port sysutils/bvm
Poudriere testport was successful: https://poudriere.ygy.io/build.html?mastername=112Ramd64-development&build=2018-07-03_18h11m43s
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Thanks for your contribution! Please see inline comments and update the diff accordingly. Also waiting for review and approval from @woodsb02 :)
| sysutils/bvm/Makefile | ||
|---|---|---|
| 1 ↗ | (On Diff #44671) | Please don't capitalize everything :) Use Created by: Qiang Guo Also, you can just use your preferred email here instead of the Phabricator one, if you want. |
| 18 ↗ | (On Diff #44671) | IMO the first file is sufficient for checking if sysutils/bhyve-firmware is installed. |
| 28 ↗ | (On Diff #44671) | You don't need these predefined paths. See comments below. |
| 33 ↗ | (On Diff #44671) | There is one predefined path ${PREFIX} (=/usr/local) which is used for this circumstance. So instead of ${STAGEDIR}${CONFDIR}, use ${STAGEDIR}${PREFIX}/etc/bvm. |
| 35 ↗ | (On Diff #44671) | Same as above. |
| 38 ↗ | (On Diff #44671) | Same as above. |
| sysutils/bvm/Makefile | ||
|---|---|---|
| 5 ↗ | (On Diff #44671) | It is PORTVERSION instead of PORTREVISION. |
Hi guoqiang, thanks for submitting this new port! I have a few comments for you before it gets committed.
Please always run "portlint -AC" on new ports to check they are compliant to a number of standard rules.
$ portlint -AC WARN: /usr/local/poudriere/ports/default/sysutils/bvm/pkg-descr: includes lines that exceed 80 characters. WARN: Makefile: [20]: whitespace before end of line. WARN: Makefile: [31]: possible use of "${CHMOD}" found. Use @(owner,group,mode) syntax or @owner/@group operators in pkg-plist instead. WARN: Makefile: possible use of absolute pathname "/usr/local/share/uef...". WARN: Makefile: possible direct use of "RUN_DEPENDS= /usr/local/share/uefi-firmware/BHYVE_UEFI.fd:sysutils/bhyve-firmware /usr/local" found. if so, use ${PREFIX} or ${LOCALBASE}, as appropriate. WARN: Makefile: extra item placed in the *_DEPENDS section, for example, "USE_GITHUB". 0 fatal errors and 6 warnings found.
Note the "extra item placed in the _DEPENDS section" warning will disappear once you have cleaned up the whitespace on line 20.
Also, please ensure you have tested the port build, and include details of how you completed that test and the result in the phabricator review. Best case this is a poudriere testport run, but if you don't have poudriere this could just be a make(1) run on the port.
You also need to edit sysutils/Makefile to add the new port in there.
| sysutils/bvm/Makefile | ||
|---|---|---|
| 17 ↗ | (On Diff #44673) | This can just be: bhyve-firmware>=0:sysutils/bhyve-firmware \ |
| 18 ↗ | (On Diff #44673) | This can just be: grub-bhyve:sysutils/grub2-bhyve \ |
| sysutils/bvm/distinfo | ||
| 4–5 ↗ | (On Diff #44673) | This shouldn't be repeated. Please delete lines 4 and 5. |
| sysutils/bvm/pkg-descr | ||
| 9 ↗ | (On Diff #44673) | Typo: s/chain/china |
| sysutils/bvm/distinfo | ||
|---|---|---|
| 4–5 ↗ | (On Diff #44673) | It's weird, I got the same result when run make makesum... |
| sysutils/bvm/pkg-descr | ||
|---|---|---|
| 9 ↗ | (On Diff #44673) | Even if the typo is fixed, I still cannot access the webpage. Please consider using a github link as the WWW. |
LGTM, except that I am not sure if the chmod line should be replaced by explicitly settings in pkg-plist (see man pkg-create). Need @woodsb02 for more input :)
| sysutils/bvm/pkg-plist | ||
|---|---|---|
| 3 ↗ | (On Diff #44838) | Use %%ETCDIR%%/bvm.conf. |
| sysutils/bvm/Makefile | ||
|---|---|---|
| 17 ↗ | (On Diff #45046) | Replace >= with >. |
| 20 ↗ | (On Diff #45046) | Need a blank line before `USE_GITHUB```. |
| 24 ↗ | (On Diff #45046) | Please silence this command with `@`. |
| 32 ↗ | (On Diff #45046) | This line isn't needed. |
| sysutils/bvm/Makefile | ||
|---|---|---|
| 32 ↗ | (On Diff #45046) | I didn't understand what you mean. Is it necessary to delete both lines 29 and 32? If you delete them, will it affect the execution of the following COPYTREE_SHARE? |
| sysutils/bvm/Makefile | ||
|---|---|---|
| 32 ↗ | (On Diff #45046) | Those lines aren't needed. |