New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 633101 link

Starred by 3 users

Issue metadata

Status: Archived
Owner:
Last visit > 30 days ago
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

go ebuild guide should not tell people to use cros-workon

Project Member Reported by vapier@chromium.org, Aug 1 2016

Issue description

the cros-workon eclass should be largely reserved for repos that are hosted on our GoB infrastructure.  using them to fetch code from external sites (like github) is bad and pointless: it wastes a lot of bandwidth doing git which slows everything down, and it can simply be switched to fetching a tarball from github's infra.
 
I'm assuming you're looking at the "Go in Chromium OS" doc on the wiki: http://www.chromium.org/chromium-os/developer-guide/go-in-chromium-os

The page explicitly tells readers to "Mirror third party repositories on Chromium Git." for third party Go packages, and the reason is the same as you mention above.

None of the example ebuilds on that page download anything directly from "github.com". Same is true for all ebuilds in chromiumos-overlay/dev-go/*. All of them get the sources from either "chromium.googlesource.com/external/..." or from "go.googlesource.com/", both of which are hosted by Google's GoB infrastructure.

sorry, it looks like i read things hastily while trying to make it through my backlog.  let me try to make up for it with some constructive thoughts ...

instead of suggesting people use git hashes, should we push for using tagged releases ?  howeyc/fsnotify for example has a number of them.  i tend to feel a lot better when i see something like:
  CROS_WORKON_COMMIT="v0.9.0"
rather than:
  CROS_WORKON_COMMIT="some-long-git-hash-arbitrarily-chosen-with-no-promise-of-QA"

i'm not sure the CROS_WORKON_TREE setting is clear.  for someone who has never looked at this before, how do they know how to compute this ?  i guess we just need to extend the info in the cros-workon.eclass comments and add an example there ?
CROS_WORKON_TREE is the hash of the git log specified by CROS_WORKON_COMMIT, it can be computed by "git log -1 --format=%T" if it's TOT, or "git log -1 --format=%T <hash_id>". Link at http://dev.chromium.org/chromium-os/chromiumos-design-docs/verify-prebuilts-using-content-hashing

Regarding vendoring/versioning of Go packages, the current state is unfortunately not very good. The version numbers you see for fsnotify package are the exception rather than the norm. Most third party Go packages on github.com/ or even on go.googlesource.com/x/ don't have version tags, and the "go get" command simply fetches the source from the tip-of-master-branch.

There's also the confusion about how to specify version numbers even in the cases they're present. Should the version be in a CROS_WORKON_COMMIT="v0.9.0" as you suggested, or should the version number be in the ebuild name (fsnotify-0.9.0.ebuild) like the Portage convention, and if it's the latter, we need extra machinery to automatically checkout the correct tag from a cloned git repository (which is probably some new logic that is not really specific to Go).

As a compromise, we can have a convention that I suggested on the fsnotify cl:

CROS_WORKON_COMMIT="441bbc86b167f3c1f4786afae9931403b99fdacf" # v0.9.0
CROS_WORKON_TREE="3038f8900106bc2b4c46b9c93f3de17a558d03b6"

where are hashes are from https://chromium.googlesource.com/external/github.com/howeyc/fsnotify/+/v0.9.0

i.e. if version tags are available in the repository, use the commit/tree hashes from one of the versioned releases, and specify the version in a comment.

i don't think extra work is needed here.  the ebuild should reflect the tag, and then we can simply use the portage variables.

so we use fsnotify-0.9.0.ebuild and then the ebuild does:
  CROS_WORKON_COMMIT="v${PV}"

that's all there is to it :).
Tested by co-existing multiple ebuild in same folder. emerge can always find the maximum versioned file to run.

For example, fsnotify/fsnotify right now is at v1.3.1. One way is to create fsnotify-1.3.1.ebuild, and create fsnotify-1.3.1-r#.ebuild for every incremental change, before next version tagged.

Creating 0.0.1.ebuild has its own benefit. For example for dev-go/dbus, it changed to 0.0.2 due to underlying package change: like below log:
> dev-go/dbus: use github.com/godbus/dbus as the source repository
>
> Previously used guelfey/go.dbus is dead/unmaintained


here's a test on go-fsnotify/fsnotify on chromium mirror.

Right now TOT is at 1.2.9.

CROS_WORKON_COMMIT="8611c35ab31c1c28aa903d33cf8b6e44a399b09e" # v1.2.9
CROS_WORKON_TREE="b590050905bc53ee3f51bff232358f1973ea6f3e"

>>> Unpacking source...
GIT update -->
   repository:               https://chromium.googlesource.com/external/github.com/go-fsnotify/fsnotify.git
   at the commit:            8611c35ab31c1c28aa903d33cf8b6e44a399b09e
   commit:                   8611c35ab31c1c28aa903d33cf8b6e44a399b09e
   branch:                   master
   storage directory:        "/var/cache/chromeos-cache/distfiles/target/egit-src/external/github.com/go-fsnotify/fsnotify"
   checkout type:            bare repository
Cloning into '/build/whirlwind/tmp/portage/dev-go/fsnotify-1.2.9/work/fsnotify-1.2.9/src/github.com/go-fsnotify/fsnotify'...
done.
Switched to a new branch 'tree-8611c35ab31c1c28aa903d33cf8b6e44a399b09e'

When I use SHA for 1.28, please see below log.

CROS_WORKON_COMMIT="508915b7500b6e42a87132e4afeb4729cebc7cbb" # v1.2.8
CROS_WORKON_TREE="792ef2dd9ca1ff73365bf48beec1bb1289418227"

>>> Unpacking source...
GIT update -->
   repository:               https://chromium.googlesource.com/external/github.com/go-fsnotify/fsnotify.git
   at the commit:            8611c35ab31c1c28aa903d33cf8b6e44a399b09e
   commit:                   508915b7500b6e42a87132e4afeb4729cebc7cbb
   branch:                   master
   storage directory:        "/var/cache/chromeos-cache/distfiles/target/egit-src/external/github.com/go-fsnotify/fsnotify"
   checkout type:            bare repository
Cloning into '/build/whirlwind/tmp/portage/dev-go/fsnotify-1.2.8/work/fsnotify-1.2.8/src/github.com/go-fsnotify/fsnotify'...
done.
Switched to a new branch 'tree-508915b7500b6e42a87132e4afeb4729cebc7cbb'

Looks like emerge is able to switch to correct commit specified by CROS_WORKON_COMMIT
Great if CROS_WORKON_COMMIT="v${PV}" just works.
Note that this implies not specifying CROS_WORKON_TREE at all in the ebuild. I'm assuming cros-workon/git eclasses are ok with that.

Zhihong, did you try Mike's suggestion with naming the ebuild fsnotify-1.2.9.ebuild and specifying

CROS_WORKON_COMMIT="v${PV}"

inside the ebuild, and removing the CROS_WORKON_TREE setting altogether?
If this works, let's use that scheme for this package.

And yes, for every incremental change to the ebuild, you'd use fsnotify-1.2.9-r#.ebuild, until the upgrade to the next revision tag.

i don't think it'll be a prob to leave CROS_WORKON_TREE blank.  the issue that var was created to mitigate doesn't exist with tagged versions.
for fsnotify, using CROS_WORKON_COMMIT="${PV}" works. Commented out CROS_WORKON_TREE or leave it blank still works, with the same log.

From the description of adding CROS_WORKON_TREE, will removing it affecting prebuilt? I'm not clear what it's for.

>>> Emerging (1 of 1) dev-go/fsnotify-1.2.9::chromiumos for /build/whirlwind/
 * Running stacked hooks for pre_pkg_setup
 *    sysroot_build_bin_dir ...                                                                                             [ ok ]
 * Running stacked hooks for pre_src_unpack
 *    python_multilib_setup ...                                                                                             [ ok ]
>>> Unpacking source...
GIT update -->
   repository:               https://chromium.googlesource.com/external/github.com/go-fsnotify/fsnotify.git
   at the commit:            8611c35ab31c1c28aa903d33cf8b6e44a399b09e
   commit:                   v1.2.9
   branch:                   master
   storage directory:        "/var/cache/chromeos-cache/distfiles/target/egit-src/external/github.com/go-fsnotify/fsnotify"
   checkout type:            bare repository
Cloning into '/build/whirlwind/tmp/portage/dev-go/fsnotify-1.2.9/work/fsnotify-1.2.9/src/github.com/go-fsnotify/fsnotify'...
done.
Switched to a new branch 'tree-v1.2.9'

>>> Emerging (1 of 1) dev-go/fsnotify-1.2.8::chromiumos for /build/whirlwind/
 * Running stacked hooks for pre_pkg_setup
 *    sysroot_build_bin_dir ...                                                                                             [ ok ]
 * Running stacked hooks for pre_src_unpack
 *    python_multilib_setup ...                                                                                             [ ok ]
>>> Unpacking source...
GIT update -->
   repository:               https://chromium.googlesource.com/external/github.com/go-fsnotify/fsnotify.git
   at the commit:            8611c35ab31c1c28aa903d33cf8b6e44a399b09e
   commit:                   v1.2.8
   branch:                   master
   storage directory:        "/var/cache/chromeos-cache/distfiles/target/egit-src/external/github.com/go-fsnotify/fsnotify"
   checkout type:            bare repository
Cloning into '/build/whirlwind/tmp/portage/dev-go/fsnotify-1.2.8/work/fsnotify-1.2.8/src/github.com/go-fsnotify/fsnotify'...
done.
Switched to a new branch 'tree-v1.2.8'



This page describes why CROS_WORKON_TREE was added: http://www.chromium.org/chromium-os/chromiumos-design-docs/verify-prebuilts-using-content-hashing

I don't think this affects ebuilds that import external Go packages.

Comment 13 Deleted

I'll need to add a dependency to fsnotify as it depends on "dev-go/go-sys". Shall I use DEPEND or RDEPEND? Searched around and looks like all ebuild go dependencies in the code base are in DEPEND. Is this because the go package is imported as source rather than library?
In this case, you should use RDEPEND. This is because fsnotify does not build a binary, it only exports a Go package. The exported Go package will work only as long as the package from dev-go/go-sys is also installed. This is ensured by RDEPEND.

This is explained in the dev-guide, although perhaps it is not very clear: http://www.chromium.org/chromium-os/developer-guide/go-in-chromium-os

-) If a dependency package is needed to build the executables, put it in DEPEND.
-) If a dependency package is imported by the Go packages installed by this ebuild, put it in RDEPEND.


Thanks! Found in the link, many notations on DEPEND/RDEPEND.

Comment 17 Deleted

so to circle back, we want to update the guide to cover the case of projects that use tags/releases and expand on the TREE variable right ?
Sounds good.
i assume you'll write those updates ? :)
Yes. I changed the status to Assigned in previous reply, and I'm the owner already :)
odd ... my e-mail & bug view didn't include that Status label change
Updated http://www.chromium.org/chromium-os/developer-guide/go-in-chromium-os
The first ebuild example is now dev-go/fsnotify, which uses ebuild version to fetch and install the corresponding tag.
Removed all usage of CROS_WORKON_TREE from dev-go/*: https://chromium-review.googlesource.com/#/c/403520
Updated all ebuild examples on the wiki page. Also added some missing documentation (variable CROS_GO_TEST).

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/223eafbf01c9bcd8646f386bda1909d70fe0a9ed

commit 223eafbf01c9bcd8646f386bda1909d70fe0a9ed
Author: Rahul Chaudhry <rahulchaudhry@chromium.org>
Date: Wed Oct 26 23:09:15 2016

dev-go: remove redundant CROS_WORKON_TREE specifications.

Specifying CROS_WORKON_TREE is not required in these ebuilds.
It is uniquely determined by CROS_WORKON_COMMIT.
Also cleaned up some DESCRIPTION and HOMEPAGE values.

BUG= chromium:633101 
TEST='sudo emerge dev-go/{dbus,fsnotify,glog,go-sys,go-tools,golint,mock,net,protobuf}' works.

Change-Id: I796fb4f8a74d47598b415d9892ca0465c5ddb668
Reviewed-on: https://chromium-review.googlesource.com/403520
Commit-Ready: Rahul Chaudhry <rahulchaudhry@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Caroline Tice <cmtice@chromium.org>

[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/go-sys/go-sys-0.0.1.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/dbus/dbus-0.0.2.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/golint/golint-0.0.1-r2.ebuild
[add] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/fsnotify/fsnotify-1.3.1-r1.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/seccomp/seccomp-9999.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/mock/mock-0.0.1.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/protobuf/protobuf-0.0.1.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/net/net-0.0.1.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/fsnotify/fsnotify-1.3.1.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/glog/glog-0.0.1-r3.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/protobuf/protobuf-0.0.1-r6.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/golint/golint-0.0.1.ebuild
[add] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/go-sys/go-sys-0.0.1-r1.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/net/net-0.0.1-r3.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/glog/glog-0.0.1.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/dbus/dbus-0.0.2-r4.ebuild
[modify] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/go-tools/go-tools-0.0.1.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/go-tools/go-tools-0.0.1-r3.ebuild
[rename] https://crrev.com/223eafbf01c9bcd8646f386bda1909d70fe0a9ed/dev-go/mock/mock-0.0.1-r2.ebuild

Status: Fixed (was: Assigned)

Comment 26 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 27 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 28 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 29 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 31 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment