Bug 219687 - [NEW PORT] sysutils/google-compute-engine: User daemon for Google Compute Engine
Summary: [NEW PORT] sysutils/google-compute-engine: User daemon for Google Compute Engine
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Some People
Assignee: Richard Gallamore
URL: https://github.com/GoogleCloudPlatfor...
Keywords: feature, needs-qa
Depends on:
Blocks:
 
Reported: 2017-05-31 16:42 UTC by Helen Koike
Modified: 2018-01-08 20:15 UTC (History)
7 users (show)

See Also:
koobs: maintainer-feedback+


Attachments
Patch which adds google-compute-engine in freebsd-ports (34.88 KB, patch)
2017-05-31 16:42 UTC, Helen Koike
no flags Details | Diff
Patch which adds google-compute-engine v2.3.7 in freebsd-ports (29.86 KB, patch)
2017-06-15 15:22 UTC, Helen Koike
no flags Details | Diff
atch which adds google-compute-engine v2.3.7 in freebsd-ports (cleaner Makefile) (28.35 KB, patch)
2017-06-15 18:12 UTC, Helen Koike
no flags Details | Diff
Patch which adds google-compute-engine v2.4.0 in freebsd-ports (29.24 KB, patch)
2017-06-28 22:51 UTC, Helen Koike
no flags Details | Diff
Patch which adds google-compute-engine v2.4.0 in freebsd-ports (29.22 KB, patch)
2017-06-29 03:11 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs of google-compute-engine package (815.94 KB, application/gzip)
2017-06-29 03:15 UTC, Helen Koike
no flags Details
Patch which adds google-compute-engine v2.4.0 in freebsd-ports (30.61 KB, patch)
2017-06-30 02:57 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs of google-compute-engine package (36.74 KB, text/x-log)
2017-07-05 21:35 UTC, Helen Koike
no flags Details
Update to version 2.4.1 (33.88 KB, patch)
2017-07-22 20:33 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs of google-compute-engine package 2.4.1 (50.00 KB, application/x-xz)
2017-07-22 20:35 UTC, Helen Koike
no flags Details
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (30.31 KB, patch)
2017-07-28 00:01 UTC, Helen Koike
no flags Details | Diff
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (32.35 KB, patch)
2017-07-28 02:06 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs of google-compute-engine package 2.4.1 (50.00 KB, application/gzip)
2017-07-28 02:07 UTC, Helen Koike
no flags Details
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (32.19 KB, patch)
2017-07-28 13:37 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs of google-compute-engine package 2.4.1 (50.00 KB, application/gzip)
2017-07-28 13:38 UTC, Helen Koike
no flags Details
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (32.16 KB, patch)
2017-07-28 13:48 UTC, Helen Koike
no flags Details | Diff
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (32.19 KB, patch)
2017-07-29 11:43 UTC, Helen Koike
no flags Details | Diff
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (31.94 KB, patch)
2017-07-29 12:28 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs of google-compute-engine package 2.4.1 (40.00 KB, application/gzip)
2017-07-29 12:28 UTC, Helen Koike
no flags Details
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (31.90 KB, patch)
2017-08-01 03:31 UTC, Helen Koike
no flags Details | Diff
Poudriere build logs (py27, py36) of google-compute-engine package (80.00 KB, application/gzip)
2017-08-01 03:32 UTC, Helen Koike
no flags Details
Patch which adds google-compute-engine v2.4.1 in freebsd-ports (32.14 KB, patch)
2017-08-01 13:05 UTC, Helen Koike
helen.koike: maintainer-approval+
Details | Diff
Poudriere build logs (py27, py36) of google-compute-engine package (90.00 KB, application/gzip)
2017-08-01 13:06 UTC, Helen Koike
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Helen Koike 2017-05-31 16:42:26 UTC
Created attachment 183109 [details]
Patch which adds google-compute-engine in freebsd-ports

Hi,

This port is meant to replace the google-daemon and google-startup-scripts package as they are really old ports (from 2014) of the project https://github.com/GoogleCloudPlatform/compute-image-packages

I am new to porting to FreeBSD, I am sending this as a new port but maybe it should just be an update of those two packages. I would appreciate if someone could review this.

I attached the patch againts freebsd-ports but you can also find it here https://git.collabora.com/cgit/user/koike/freebsd-ports.git/log/?h=upstreaming

Thanks

Port description:
    net/google-compute-engine: user daemon for Google Compute Engine

    Google Compute Engine offers scripts and daemons which runs in the
    background and provides the following services:

    - Accounts daemon to setup and manage user accounts, and to enable SSH key based authentication.
    - Clock skew daemon to keep the system clock in sync after VM start and stop events.
    - Instance setup scripts to execute VM configuration scripts during boot.
    - IP forwarding daemon that integrates network load balancing with forwarding rule changes into the guest.
    - Metadata scripts to run user provided scripts at VM startup and shutdown.
    - Network setup service to enable multiple network interfaces on boot.
Comment 1 Helen Koike 2017-06-15 15:22:42 UTC
Created attachment 183499 [details]
Patch which adds google-compute-engine v2.3.7 in freebsd-ports

Hi,

I updated the port to include version 2.3.7 of the port and marked the old one (version 2.3.6) as obsolete.
In this version I also erased the pkg-plist file as I am using autoplist

I would appreciate if anyone could review it.

Thanks
Comment 2 Helen Koike 2017-06-15 18:12:56 UTC
Created attachment 183507 [details]
atch which adds google-compute-engine v2.3.7 in freebsd-ports (cleaner Makefile)

Hi,

I removed lang/python from Makefile as it is already listed in USE=python
I also reduced the SHEBANG_FILE list by using SHEBANG_GLOB
Thanks to Johannes Meixner for suggesting these changes.

NOTE: the rc script google_shutdown_scripts.in has an empty start function. I could merge this script with google_startup_scripts.in, but I left them separated to preserve the style from the upstream project that has both e.g. https://github.com/GoogleCloudPlatform/compute-image-packages/tree/master/google_compute_engine_init/sysvinit

Please, see new attachment.
Thanks
Comment 3 Helen Koike 2017-06-28 22:51:08 UTC
Created attachment 183895 [details]
Patch which adds google-compute-engine v2.4.0 in freebsd-ports

Hi,

I updated the package to version 2.4.0, and also removed the unecessary shebang fix as setuptools already does it.

I would appreciate if anyone could review this.
Please, see the attached package.

Thanks
Comment 4 Richard Gallamore freebsd_committer freebsd_triage 2017-06-29 00:37:36 UTC
Hey there Helen, thanks for submitting the port. After an initial scan, I have found a few items that need to be addressed.

 - Upstream versioning is by date. Why does this not match upstream?
 - Add license.
 - SUB_LIST is in USE section, should be after option section and before target section.
 - In pkg-descr, WWW, please list a site with plenty of documentation and information about the port. The git repo is lacking heavily on this.


Can you also please post a portlint -AC and attach a poudriere build logs if possible.
Comment 5 Helen Koike 2017-06-29 03:11:51 UTC
Created attachment 183898 [details]
Patch which adds google-compute-engine v2.4.0 in freebsd-ports

(In reply to Richard Gallamore from comment #4)

Hi Richard, thanks for reviewing this package, I am attaching a new version with the suggested changes and a few other that I found in portlint (thanks for suggesting this, I am still getting familiar with all the FreeBSD tools)

> Hey there Helen, thanks for submitting the port. After an initial scan, I have
> found a few items that need to be addressed.
> 
>   - Upstream versioning is by date. Why does this not match upstream?

This match upstream, it is just the github tag that is by date, the version is in the setup.py file as shown here https://github.com/GoogleCloudPlatform/compute-image-packages/commit/5824f1bba1906f6bb298db2efaf1bffc0671ff45

>   - Add license.

Done

>   - SUB_LIST is in USE section, should be after option section and before target
> section.

Done, I added just before "post-patch:" target, is it ok now?

>   - In pkg-descr, WWW, please list a site with plenty of documentation and
> information about the port. The git repo is lacking heavily on this.

I modified this to point to the main page where the docs are https://github.com/GoogleCloudPlatform/compute-image-packages/
I think this is the most complete docs available

> 
> 
> Can you also please post a portlint -AC and attach a poudriere build logs if
> possible.
> 

I also adjusted pkg-desc so lines don't exceed 80 characters
And I removed DISTVERSION (as it cannot be user with PORTVERSION)

    -PORTVERSION=   2.4.0
    -DISTVERSION=   20170609
    +PORTVERSION=   20170609

But now it uses version 20170609 instead of 2.4.0, is this ok? I feel that 2.4.0 should be somewhere as it is the version used in setup.py, what do you think?
I tried to use GH_TAGNAME=20170609 with PORTVERSION=2.4.0 but it doesn't seem to work the way I imagined.

With these changes now portlint returns:
# portlint -AC
looks fine.

I'm attaching the poudriere logs in the comment that follows

Thanks for your help
Comment 6 Helen Koike 2017-06-29 03:15:20 UTC
Created attachment 183899 [details]
Poudriere build logs of google-compute-engine package

I am adding the poudriere build logs of the google-compute-engine. All packages and dependencies built successfully.

I am new to this tool, let me know if there is any special flags you want me to add be more verbose or to do more tests.

Thanks
Comment 7 Richard Gallamore freebsd_committer freebsd_triage 2017-06-29 09:10:53 UTC
(In reply to Helen Koike from comment #5)
> This match upstream, it is just the github tag that is by date, the version is in the setup.py file as shown here > https://github.com/GoogleCloudPlatform/compute-image-packages/commit/5824f1bba1906f6bb298db2efaf1bffc0671ff45

I think the other versioning would be better, please change it back. I just wanted to verify correct versioning is being use.

> Done, I added just before "post-patch:" target, is it ok now?
Perfect.

> I also adjusted pkg-desc so lines don't exceed 80 characters
> And I removed DISTVERSION (as it cannot be user with PORTVERSION)
There isn't really a character limit on pkg-descr. This looks descriptive enough to me. 

> But now it uses version 20170609 instead of 2.4.0, is this ok? I feel that 2.4.0 should be somewhere as it is the version used in setup.py, what do you think?
> I tried to use GH_TAGNAME=20170609 with PORTVERSION=2.4.0 but it doesn't seem to work the way I imagined
Yes, revert back to how it was previously. This was a better solution, just needed to verify.

> With these changes now portlint returns:
> # portlint -AC
> looks fine.
This is great.


Couple things with the poudriere log.
- Only the port listed in summary is required, in this case net/google-compute-engine.
- Not sure how the build was invoked, but I usually use poudriere bulk -tC.(-t is for extra testing and -C will clean the specified package before build)
I also want to note that poudriere bulk is not the recommended procedure. From the porters handbook, testport is suggested method, details here[1].


When I initially went to the github repo, for some odd reason the README.md did not show up and was just blank with no information. Going back to it now, the information I was expecting is shown, so this work perfectly for WWW. There are some other items that need to be will need to be looked at.

- Portname. Usually this is the same as project name, but I don't think that would be a good fit. google-compute-image would be a bit more accurate, but more opinions would be best.
- The comment should match the project comment on github minus "Linux". Changing this however with the current portname is a violation of having the portname in comment so waiting for portname feedback before deciding the correct comment.
- pkg-message.in[2]. Can you please provide a simple setup guide. It doesn't need to be long, just a simple how-to startup guide of procedures required after installing the package for the first time. If configuration is required, pointing to a url with more information would be great.

One more bit of information, how was the port tested? or is the port currently being used in production?

Thanks for your efforts on this port!

[1] https://www.freebsd.org/doc/en/books/porters-handbook/book.html#testing-poudriere
[2] https://www.freebsd.org/doc/en/books/porters-handbook/book.html#porting-message
Comment 8 Kubilay Kocak freebsd_committer freebsd_triage 2017-06-29 11:04:53 UTC
Thank you for your new port contribution Helen.

Pending further review, initial review items (needing updated patch) are:

- Add missing PKGNAMEPREFIX=${PYTHON_PKGNAMEPREFIX}
- Missing secondary 'python' virtual category in CATEGORIES 
- Add USE_PYTHON=concurrent if the port is multiple python version concurrent installation safe (all files are uniquely named and dont conflict)
- Use 'CHEESESHOP' (PyPI) as MASTER_SITES unless there is a compelling reason not to (for instance, test or other important files missing from source distribution on PyPI)
- Add test support (using do-test: target), adding TEST_DEPENDS (py.test, etc as per setup.py/tox.ini) and executing ${PYTHON_CMD} -m pytest or ${PYTHON_CMD} setup.py test if supported. It's OK if not all test dependencies have been ported yet, just add the ones that have, adding comments for the ones that havent.
- setuptools appears (in setup.py) as an install_requires (RUN_DEPENDS) not a setup_requires (BUILD_DEPENDS) as in the patch. I don't know why its explicitly in install_requires though.
- Add LICENSE_FILE=${WRKSRC}/path/to/license when one exists in the distribution file
- netifaces/netaddr don't appear as setup.py (compulsory) or tox.ini (potentially optional) dependencies. Can you elaborate on their source and function?

If possible (highly desirable/recommended), please provide as an attachment the results of the package test suite as well.

For more information on creating ports for Python packages, see:

https://wiki.freebsd.org/Python/PortsPolicy
Comment 9 Helen Koike 2017-06-29 21:26:37 UTC
(In reply to Richard Gallamore from comment #7)

>> But now it uses version 20170609 instead of 2.4.0, is this ok? I feel that 2.4.0 should be somewhere as it is the version used in setup.py, what do you think?
>> I tried to use GH_TAGNAME=20170609 with PORTVERSION=2.4.0 but it doesn't seem to work the way I imagined
> Yes, revert back to how it was previously. This was a better solution, just
> needed to verify.
> 
>> With these changes now portlint returns:
>> # portlint -AC
>> looks fine.
> This is great.

The problem to revert it back I get:
# portlint -AC
FATAL: Makefile: either PORTVERSION or DISTVERSION must be specified, not both.

What is the best way to handle this?

> Couple things with the poudriere log.
> - Only the port listed in summary is required, in this case
> net/google-compute-engine.
> - Not sure how the build was invoked, but I usually use poudriere bulk -tC.(-t
> is for extra testing and -C will clean the specified package before build)
> I also want to note that poudriere bulk is not the recommended procedure. From
> the porters handbook, testport is suggested method, details here[1].

Thanks, I'll regenerate the poudriere tests like you suggested.

> 
> 
> When I initially went to the github repo, for some odd reason the README.md did
> not show up and was just blank with no information. Going back to it now, the
> information I was expecting is shown, so this work perfectly for WWW. There are
> some other items that need to be will need to be looked at.
> 
> - Portname. Usually this is the same as project name, but I don't think that
> would be a good fit. google-compute-image would be a bit more accurate, but
> more opinions would be best.

The github project is called compute-image-packages but it provides 3 different packages as described at https://github.com/GoogleCloudPlatform/compute-image-packages#packaging.
	- google-compute-engine
	- google-compute-engine-init
	- google-config
So I believe that we should have one port for each of them.
google-compute-engine-init is not necessary, as it only provides init scripts but I already added rc scripts as .in files in the google-compute-engine package.
google-config provide some udev rules, syslog configuration and bash scripts for hostname and dhcp that are not really required for google-compute-engine but I intend to port google-config latter.

So as there are 3 packages under the github compute-image-packages project, I am porting just the google-compute-engine one, so I think it should keep this name.

> - The comment should match the project comment on github minus "Linux".
> Changing this however with the current portname is a violation of having the
> portname in comment so waiting for portname feedback before deciding the
> correct comment.

Do you mean "Guest Environment for Google Compute Engine" ?
It could be, I am just wondering which should be the Comment in the future google-config package

> - pkg-message.in[2]. Can you please provide a simple setup guide. It doesn't
> need to be long, just a simple how-to startup guide of procedures required
> after installing the package for the first time. If configuration is required,
> pointing to a url with more information would be great.

The easier configuration is a reboot to make the init scripts to start, I'll write this in pkg-message and I can add a manual guide.

> 
> One more bit of information, how was the port tested? or is the port currently
> being used in production?

I am testing it by hand in a virtual machine running at Google Cloud Engine platform.
The available tests in the upstream project 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 FreeBSD (as it is implemented in the project), I don't think we need to bother to port them. What do you think?


---
(In reply to Kubilay Kocak from comment #8)

Thank you Kubilay for reviewing this, I am updating the package with the suggested changes and I'll attach it soon.
Comment 10 Helen Koike 2017-06-30 02:57:00 UTC
Created attachment 183934 [details]
Patch which adds google-compute-engine v2.4.0 in freebsd-ports

(In reply to Kubilay Kocak from comment #8)

> Thank you for your new port contribution Helen.
> 
> Pending further review, initial review items (needing updated patch) are:

Thanks for your review
I applied most of the items, please see new attached patch. But I still have some questions below.

> 
> - Add missing PKGNAMEPREFIX=${PYTHON_PKGNAMEPREFIX}

Done

> - Missing secondary 'python' virtual category in CATEGORIES

Done

> - Add USE_PYTHON=concurrent if the port is multiple python version concurrent
> installation safe (all files are uniquely named and dont conflict)

Done

> - Use 'CHEESESHOP' (PyPI) as MASTER_SITES unless there is a compelling reason
> not to (for instance, test or other important files missing from source
> distribution on PyPI)

I added CHEESESHOP, but do I need to define MASTER_SITES when USE_GITHUB=yes ?

> - Add test support (using do-test: target), adding TEST_DEPENDS (py.test, etc
> as per setup.py/tox.ini) and executing ${PYTHON_CMD} -m pytest or ${PYTHON_CMD}
> setup.py test if supported. It's OK if not all test dependencies have been
> ported yet, just add the ones that have, adding comments for the ones that
> havent.

setup.py test is not supported, as explained in the previous comment, only mock tests are supported and I am not sure if it is worthy to port them all.

> - setuptools appears (in setup.py) as an install_requires (RUN_DEPENDS) not a
> setup_requires (BUILD_DEPENDS) as in the patch. I don't know why its explicitly
> in install_requires though.

hm, I'll check with upstream maintainers, it centenly doesn't use setuptools at run time

> - Add LICENSE_FILE=${WRKSRC}/path/to/license when one exists in the
> distribution file

Done

> - netifaces/netaddr don't appear as setup.py (compulsory) or tox.ini
> (potentially optional) dependencies. Can you elaborate on their source and
> function?

The upstream project doesn't use netifaces/netaddr.
For example, it uses /sys/class/net/<iface-name>/address to retrieve the mac address. As FreeBSD doesn't seem to have this patch in sysfs, I changed the script to use netifaces/netaddr instead and I added them in the dependencies list

> 
> If possible (highly desirable/recommended), please provide as an attachment the
> results of the package test suite as well.

As the mock tests are not ported, I am testing by hand so I unfortunatelly doesn't have a test log to attach.

> 
> For more information on creating ports for Python packages, see:
> 
> https://wiki.freebsd.org/Python/PortsPolicy

Thanks for this link, I also add the following changes:

* I set the PORTVERSION back to 2.4.0 but I had to define GH_TAGNAME and DISTNAME so it downloads the package correctly, is this ok?
-PORTVERSION=   20170609
...
+PORTVERSION=   2.4.0
+DISTNAME=      ${GH_ACCOUNT}-${GH_PROJECT}-${GH_TAGNAME}
...
+GH_TAGNAME=    20170609

* About the patches, I know they are not ready to be upstreamable yet (it is not that straight forward to make it compatible to Linux), but I'll work on that. Should I add comments on the patches anyway?

* Portlint result:
# portlint -AC
WARN: Makefile: using hyphen in PORTNAME. consider using PKGNAMEPREFIX and/or PKGNAMESUFFIX.
WARN: /usr/ports/net/google-compute-engine/pkg-message: possible use of absolute pathname "/etc/rc.conf".
0 fatal errors and 2 warnings found.

It seems two false positives, the first one wasn't firing before, not sure why it is appearing now. About the second one, I checked how other packages mention /etc/rc.conf and all of them do like this, so I suppose this is the right way to do.

Thanks
Comment 11 Helen Koike 2017-07-05 21:35:10 UTC
Created attachment 184099 [details]
Poudriere build logs of google-compute-engine package

Sorry, I forgot to attach the poudriere log, I am attaching it now.

It was generated through the command:

    poudriere testport -j 11amd64 -p local -o net/google-compute-engine

Thanks
Comment 12 Helen Koike 2017-07-22 20:33:41 UTC
Created attachment 184600 [details]
Update to version 2.4.1

Hi,

I attaching a new patch updating to version 2.4.1.

Could someone review this please?

Thanks
Comment 13 Helen Koike 2017-07-22 20:35:45 UTC
Created attachment 184601 [details]
Poudriere build logs of google-compute-engine package 2.4.1
Comment 14 Richard Gallamore freebsd_committer freebsd_triage 2017-07-22 21:25:09 UTC
(In reply to Helen Koike from comment #12)
This port looks good to me. One thing though, there is a second port attached at the bottom. Should be a separate PRs for each port. If this port depends on it, it should be added to Depends on: and the PR number.


In that port, I noticed the comment has Linux guest, this should be changed to Unix as FreeBSD isn't Linux.

USES should start the USES, USE_x section.


Python is Koobs domain. I don't think it is necessary to have all the mock tests working, but if he believes it should be done then I won't proceed unless the thumbs up are given by him or the tests are added and successful.


Thanks again for your contributions on this Helen, it is looking great!
Comment 15 Po-Chuan Hsieh freebsd_committer freebsd_triage 2017-07-22 21:33:24 UTC
(In reply to Helen Koike from comment #12)

I noticed 2 ports in attachment 184600 [details].
Since we use shar for adding new ports, could you please create .shar files for each port and resubmit it?
Thanks!
Comment 16 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-23 08:48:59 UTC
(In reply to Richard Gallamore from comment #14)

Tests working (or at least a target created so that they can be run/seen, even if their are failures) are not a blocker, but highly desirable.
Comment 17 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-23 09:06:55 UTC
- Patches should be commented/referenced (upstream issues/commits) where possible
- Port should use python framework components (use=python, use_python, etc)

Also, rationale/more information on the following would be useful:

- A separate google-config port with no dependency registered on it. Could this (google-compute-engine) port install the configs itself instead?

- References to rsyslog / udev, without associated dependencies on rsyslog or libudev (if needed). If these configs are being modified to work with base freebsd functionality and installed in base directories, are they just called rsyslog/udev  to stick with upstream naming of modules?

- Installation and integration of files into base directories. The need for this (installing into base) should be actual, compelling and well tested, or alternatives implemented.

Given the above complexity I wont be able to review it in the detail that it appears to need at the moment. If the port is modified to be a standard (albeit even substantially modified) python package, please feel free to re-assign to me.

I'm happy to continue to review python specific issues/questions on request
Comment 18 Steve Wills freebsd_committer freebsd_triage 2017-07-24 18:43:43 UTC
(In reply to Po-Chuan Hsieh from comment #15)
Although they used to be, shar files are no longer required for new ports. Instead, both shar files and patch/diff files are acceptable. Please see:

https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/porting-submitting.html
Comment 19 Helen Koike 2017-07-28 00:01:36 UTC
Created attachment 184781 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

Sorry, the second port google-config shouldn't be in the patch, it was my mistake when extracting the diff.

I am re-attaching the patch without the google-config.

The google-config is not ready yet, not well tested and I think it would be better to merge it in the google-compute-engine package instead.

Thanks a lot
Comment 20 Helen Koike 2017-07-28 02:06:38 UTC
Created attachment 184782 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

I normalized the patches and added comments on the top.
I also checked about the dependency of setuptools and I confirm that I need it in RUN_DEPENDS, as it is used by the generated scripts, they import pkg_resources provided by the setuptools package.

Please, see the new attached patch.

Thanks
Comment 21 Helen Koike 2017-07-28 02:07:17 UTC
Created attachment 184783 [details]
Poudriere build logs of google-compute-engine package 2.4.1
Comment 22 Po-Chuan Hsieh freebsd_committer freebsd_triage 2017-07-28 02:30:20 UTC
I did a quick review.

Please remove the following from Makefile:
- MASTER_SITES: CHEESESHOP does not have the tarball. USE_GITHUB will add GitHub to MASTER_SITES.
- DISTNAME: it's set by USE_GITHUB.
- BUILD_DEPENDS: it's already added by USES=python and USE_PYTHON=distutils.
- setuptools in RUN_DEPENDS: it's already added by USES=python and USE_PYTHON=distutils.

After the removal:
% make -V MASTER_SITES
https://codeload.github.com/GoogleCloudPlatform/compute-image-packages/tar.gz/20170718?dummy=/
Comment 23 Helen Koike 2017-07-28 13:37:55 UTC
Created attachment 184798 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

(In reply to Po-Chuan Hsieh from comment #22)

Thanks for your review.
I remove the items you mentioned but the DISTNAME, because USE_GIT adds the PORTVERSION in the DISTNAME and it gives me the following error:

    => GoogleCloudPlatform-compute-image-packages-2.4.1-20170718_GH0.tar.gz is not in /usr/ports/net/google-compute-engine/distinfo.

While in distinfo I have GoogleCloudPlatform-compute-image-packages-20170718_GH0.tar.gz instead (without PORVERSION 2.4.1)
Comment 24 Helen Koike 2017-07-28 13:38:39 UTC
Created attachment 184799 [details]
Poudriere build logs of google-compute-engine package 2.4.1
Comment 25 Helen Koike 2017-07-28 13:48:30 UTC
Created attachment 184800 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

Sorry, I misunderstood and I hadn't completely removed MASTER_SITES from the Makefile. re-attaching the patch without MASTER_SITES.
Comment 26 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-29 07:18:49 UTC
(In reply to Po-Chuan Hsieh from comment #22)

PyPI does have the tarball: https://pypi.python.org/pypi/google-compute-engine

google-compute-engine-2.4.0.tar.gz (md5)
Comment 28 Helen Koike 2017-07-29 12:28:03 UTC
Created attachment 184830 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

Sorry, the previous patch is incorrect. Attaching a fixed version.
Sorry about the noise (I am still learning this)
Comment 29 Helen Koike 2017-07-29 12:28:24 UTC
Created attachment 184831 [details]
Poudriere build logs of google-compute-engine package 2.4.1
Comment 30 Po-Chuan Hsieh freebsd_committer freebsd_triage 2017-07-29 12:52:48 UTC
(In reply to Kubilay Kocak from comment #26)

Oh, my apologies. I removed DISTNAME first, therefore it failed to fetch the tarball from PyPI.
Comment 31 Po-Chuan Hsieh freebsd_committer freebsd_triage 2017-07-29 13:20:21 UTC
(In reply to Helen Koike from comment #28)

1. Either remove PKGNAMEPREFIX=${PYTHON_PKGNAMEPREFIX} or change this port as net/py-google-compute-engine.

We add py- to the directory name and pyXX- to package name for Python libraries/modules.
Please consider if this port is more of a library or an application.

2. We have ntpdate in base system (/usr/sbin/ntpdate). Is there any reason to use the one from ports?

3. There is compilation error with Python 3. Please add a patch for it.

byte-compiling /usr/ports/works/usr/ports/net/google-compute-engine/work/stage/usr/local/lib/python3.6/site-packages/google_compute_engine/network_utils.py to network_utils.cpython-36.pyc
  File "/usr/local/lib/python3.6/site-packages/google_compute_engine/network_utils.py", line 47
    except Exception, e:
                    ^
SyntaxError: invalid syntax

4. Please sort RUN_DEPENDS and USE_PYTHON.

5. Use REINPLACE_ARGS=-i '' to avoid saving backup files.

"@${REINPLACE_CMD} -i -e" in post-patch: will save backup files with "-e" suffix.

% ls -l google_compute_engine/network_setup/network_setup.py*
-rwxr-xr-x  1 sunpoet  wheel  6454 Jul 29 20:52 google_compute_engine/network_setup/network_setup.py
-rwxr-xr-x  1 sunpoet  wheel  6454 Jul 19 00:49 google_compute_engine/network_setup/network_setup.py-e

It should be "REINPLACE_ARGS=-i ''" to avoid saving backup files.

% grep REINPLACE_ARGS /usr/ports/Mk/bsd.port.mk
REINPLACE_ARGS?=        -i.bak
REINPLACE_CMD?= ${SED} ${REINPLACE_ARGS}

Thanks.
Comment 32 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-29 13:34:05 UTC
(In reply to Po-Chuan Hsieh from comment #31)

Re (1), the reason to only prefix python modules/libraries is outdated, if it was a useful to begin with. Prefixing is required so that that multiple Python versions of the same package can be installed. Less important is whether the directory is prefixed, but still preferred for consistency. tldr; there should be a compelling case not to prefix, but being a module, library or not is no longer one of them
Comment 33 Helen Koike 2017-08-01 03:31:14 UTC
Created attachment 184878 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

I am attaching a new version of the package with these corrections:

(In reply to Po-Chuan Hsieh from comment #31)

Re (1): I removed the py prefix then, as this is more an application/service then a module, and there shouldn't be different installed versions of the package using different python versions.

Re (2): I removed ntpdate (I confused myself with OpenBSD where the rc script is provided by the ports), but it doesn't seem to be the case of FreeBSD

Re (3, 4, 5): Done
Comment 34 Helen Koike 2017-08-01 03:32:32 UTC
Created attachment 184879 [details]
Poudriere build logs (py27, py36) of google-compute-engine package

I added the poudriere logs for python 27 and also for python 36 this time
Comment 35 Kubilay Kocak freebsd_committer freebsd_triage 2017-08-01 10:13:18 UTC
As per comment 32, the 'directory name' may be prefix-less (but should be prefixed as a default), but the pkgnameprefix must be retained. We cannot control what versions of python a port/package is built with, we can only control whether they conflict or not (the reason for the prefix is so they dont)
Comment 36 Helen Koike 2017-08-01 13:05:42 UTC
Created attachment 184909 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

Attaching a new version.
Changes:
* Package moved to sysutils as it seems more precise then net
* Re-add PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX} as before
* Prefix directory name: py-google-compute-engine
Comment 37 Helen Koike 2017-08-01 13:06:21 UTC
Created attachment 184910 [details]
Poudriere build logs (py27, py36) of google-compute-engine package
Comment 38 Helen Koike 2017-08-16 14:56:52 UTC
Comment on attachment 184909 [details]
Patch which adds google-compute-engine v2.4.1 in freebsd-ports

Hi,

I think this package is ready to be commited, could anyone please commit it?

Thanks
Comment 39 Richard Gallamore freebsd_committer freebsd_triage 2017-08-18 23:58:42 UTC
The only thing left that I can see is NO_ARCH= yes. Thanks for your contributions Helen, wrap this up.
Comment 40 Richard Gallamore freebsd_committer freebsd_triage 2017-08-19 00:11:14 UTC
Will have its final review here[1] if anyone is interested in following/reviewing further.

[1] https://reviews.freebsd.org/D12077
Comment 41 commit-hook freebsd_committer freebsd_triage 2017-08-19 18:03:56 UTC
A commit references this bug:

Author: ultima
Date: Sat Aug 19 18:02:58 UTC 2017
New revision: 448349
URL: https://svnweb.freebsd.org/changeset/ports/448349

Log:
  Google Compute Engine offers scripts and daemons which runs in the
  background and provides the following services:

  - Accounts daemon to setup and manage user accounts, and to enable SSH key based
    authentication.
  - Clock skew daemon to keep the system clock in sync after VM start and stop
    events.
  - Instance setup scripts to execute VM configuration scripts during boot.
  - IP forwarding daemon that integrates network load balancing with forwarding
    rule changes into the guest.
  - Metadata scripts to run user provided scripts at VM startup and shutdown.
  - Network setup service to enable multiple network interfaces on boot.

  WWW: https://github.com/GoogleCloudPlatform/compute-image-packages

  PR:		219687
  Submitted by:	Helen Koike (maintainer)
  Reviewed by:	matthew (mentor), koobs, sunpoet, swills, julian
  Approved by:	matthew (mentor)
  Differential Revision:	https://reviews.freebsd.org/D12077

Changes:
  head/sysutils/Makefile
  head/sysutils/py-google-compute-engine/
  head/sysutils/py-google-compute-engine/Makefile
  head/sysutils/py-google-compute-engine/distinfo
  head/sysutils/py-google-compute-engine/files/
  head/sysutils/py-google-compute-engine/files/google_accounts_daemon.in
  head/sysutils/py-google-compute-engine/files/google_clock_skew_daemon.in
  head/sysutils/py-google-compute-engine/files/google_instance_setup.in
  head/sysutils/py-google-compute-engine/files/google_ip_forwarding_daemon.in
  head/sysutils/py-google-compute-engine/files/google_network_setup.in
  head/sysutils/py-google-compute-engine/files/google_startup.in
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_accounts_accounts__daemon.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_accounts_accounts__utils.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_boto_boto__config.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_boto_compute__auth.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_clock__skew_clock__skew__daemon.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_config__manager.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_instance__setup_instance__config.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_instance__setup_instance__setup.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_ip__forwarding_ip__forwarding__daemon.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_ip__forwarding_ip__forwarding__utils.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_metadata__scripts_script__executor.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_network__setup_network__setup.py
  head/sysutils/py-google-compute-engine/files/patch-google__compute__engine_network__utils.py
  head/sysutils/py-google-compute-engine/files/patch-setup.py
  head/sysutils/py-google-compute-engine/pkg-descr
  head/sysutils/py-google-compute-engine/pkg-message
Comment 42 Richard Gallamore freebsd_committer freebsd_triage 2017-08-19 18:14:20 UTC
The rc scripts being default on are not acceptable and had to be changed to default off. I also adjusted the pkg-message. The startup/shutdown scripts were also merged. If you would like to change any of these, please open a new PR with the changes.

Also want to mention that julian on the differential mentioned a couple things and I suggest taking a look.


Anyways, committed! thanks everyone.