Re: [NEW] net/google-compute-engine

[Helen Koike]

Hi Stuart,

Thanks for your review. Please, check my comments below.



On 2017-06-02 05:46 AM, Stuart Henderson wrote:

I also attached a new version with most of the suggested changes (and upstream version 2.3.7 instead of 2.3.6), I would appreciate if you could take a look if it is ok now.

On 2017/06/01 23:58, Helen Koike wrote:

1. $ portcheck
patches/patch-google__compute__engine_accounts_accounts__daemon.py does not 
have $OpenBSD$ RCS tag at the top
[...]

I guess you don't know about make update-patches' do you? :)

no, I don't :(
(yet) I am in the process of "s/don't/do" in this phrase

Patch filenames shouldn't have "." either. I suggest this to normalize
them all:

make clean
make patch
mv patches patches.old     (or just rm patches/*)
make update-patches

This works great, thanks.

If you patch any other files, "cp somefile somefile.orig", edit somefile,
"make update-patches" to pick it up.

6. does it really need net/ntp as a dependency?
google_compute_engine/clock_skew/clock_skew_daemon.py:    ntpd_inactive = 
subprocess.call(['/etc/rc.d/ntpd', 'check'])
That rather smells like openntpd should be enough :)

ok, I'll check openntpd

/etc/rc.d/ntpd is for openntpd, which is in the base OS.
net/ntpd in ports is the ntp.org software and only used for special
cases on OpenBSD, it's not a good choice for general-purpose ntp here.

The problem is that openntpd doesn't provide ntpdate, is there another way to force openntp to update the time? Just restarting the service doesn't work

6bis. How did you choose the actual list of dependencies?

I checked which programs and python libs the code was using and I added
in the list basically. But I think I missed some libs, I'll check/test
again.

I haven't looked but hopefully there will be a list in setup.py
that will help.

In setup.py there is only "install_requires=['boto', 'setuptools']" :(

7. Any reason to not to enable tests?

no (I just didn't know I had this option), I'll check that

You can do this as a do-test target in the port's Makefile.

The available tests are just mock tests that verifies if the software calls the right functions in the right order, it doesn't really check if it works or not and it can be biased as it was derivated directly from the code. To enable the tests I'll also need to port each test, increasing the number of patches and the maintenance complexity. As I believe the mock tests are more important to the developer then to OpenBSD (as it is implemented in the project), I don't think we need to bother to port them.

9. What's the point of patching stuff like
-      '/usr/bin/google_instance_setup.')
+      '%%PREFIX%%/bin/google_instance_setup.')
and then sed'ding it in pre-configure?

Because I saw other ports doing this, and this allows different
installation PREFIX to be used no?

The usual method is to patch to use ${LOCALBASE} and use something like
"${SUBST_CMD} ${WRKSRC}/dir/file" in the port's Makefile. Ones with %%
etc are often old or there's some special reason.

This used to be a bit annoying and undid the ${..} changes if you ran
"update-patches" after a build, workaround is to clean/patch/edit/update
without building, but this has been improved after the 6.1 release.

10. LOCALBASE wouldn't be better than PREFIX?

maybe, I didn't know about it, I'll check, thanks

PREFIX is correct for that one. (PREFIX is "file from this port being
installed", LOCALBASE is "file from another port that we're referencing")

done

11. Don't you need a RDEP on net/py-netifaces?

Yes, It is there already:

RUN_DEPENDS =   shells/bash \
                security/sudo \

Prefer to use doas (in the base OS) rather than sudo if possible.

done

Also prefer to use ksh from base rather than bash if the scripts work
with it without excessive changes.

done

For the rc scripts, I suggest a "meta" script (google_compute_engine.rc?)
which calls the others and refer to that in the README, so users only have
to make one entry in pkg_scripts and don't need to worry about ordering.
sysutils/bacula/pkg/bacula.rc would be a good model.

awesome suggestion, thanks for that

nit: the === underlining in pkg/README is too short, make sure you edit
it with a fixed-width font.

Could you re-check if it is ok now? Thanks


Here is a list of changes the attached version:
- upstream version update to 2.3.7 (and update distinfo)
- remove HOMEPAGE from Makefile (already set in GH)
- remove bash as dependency (use ksh instead)
- remove sudo (use doas instead)
- use SUBST_CMD in pre-configure
- normalize patches
- fix bug in network_setup.py in sed command (wrong syntax)
- add readme in PLIST
- add meta rc script called google_compute_engine
- update README


Thanks a lot
LN Koike

google-compute-engine.tar.gz
Description: application/gzip