Note: using goimports directly may not work, because the presubmit check could run outside of the chroot, which is different from the build environment.

Though, focusing on import "order" only, we may be able to parse each .go file, and checking the import order itself manually may work.
Ensuring not to add unneeded imports and the resolution of all imports should be covered by actual tast run.

Thanks for filing this. Running "goimports -local chromiumos/" in a pre-upload hook and comparing the output to the actual file may be enough. I believe that this would replace replace the existing _check_gofmt hook, since goimports also does formatting.

Didn't see your comment when I wrote mine. Is the concern that the version of goimports on the developer's system may differ from the one in the chroot?

My concern is something similar to "go vet" thing we discussed separately;

IIUC, goimports cleans up, e.g., unused imports, which leads compile error in golang.
It requires to parse the source, and also semantic analysis. TIL, the package name may not be the basename of the import path, so it is necessary to take a look at pointed .go files or something to figure out if the package is actually used or not.
That means, the build environment which is as same as what we have in chroot is needed to run it.
I haven't investigated it yet in deep, but at least goimports does not seem to suppport "FormatOnly" option for imports.Process().

Stepping back, what we'd like to have here is not those insertion/deletion of imports by resolving,
but just fixing the order. So, my proposal here is to create a small program to reorder imports (probably thin wrapper of imports.Process, I'm imagining).
WDYT?

Hmm, I'm still not sure that this is an issue. I run goimports in my editor outside of my chroot (and suspect that most other developers will too) and haven't noticed any problems with it behaving incorrectly.

It's unable to automatically add imports for chromiumos packages (e.g. if I add a reference to "chrome" in a file, I need to add the import manually), but I think it will sort imports (and also drop unneeded imports), which is the main goal here IIUC.

So I'm still wondering if running goimports and failing if its output differs from the original file might be enough.

I'm still trying to figure out how to replicate our desired configuration in a Google-internal Vim configuration. Per our earlier IM discussion (which is no longer available :-/), I think that you discovered that my configuration was running "goimports" once without any args and then running it a second time with "-local chromiumos/", which yielded different results than just running it once with "-local chromiumos/". Is that correct?

When I run "goimports --local chromiumos/ chrome.go" on my workstation right now, I see this right now:

import (
        "context"
...
        "time"

        "chromiumos/tast/crash"
...
        "chromiumos/tast/testing"

        "github.com/godbus/dbus"

        "github.com/mafredri/cdp/devtool"
)

That's different from the order being applied by https://crrev.com/c/1270538, though.

Am I misunderstanding? What command(s) did you run to reformat the code for that change?

What I "guessed" is (yes i don't have local repro env as I'm emacs user), as you said,
running goimports without "--local chromiumos/" then "goimports --local chromiumos/",
which produces what your vim produces.

Starting from import without empty line separation, i.e.;

import (
    "context"
...
    "time"
    "chromiumos/tast/crash"
...
    "chromiumos/tast/testing"
    "github.com/godbus/dbus"
    "github.com/mafredri/cdp/devtool"
)

the first command would make two groups. 1) stdlib and chromiumos, and 2) third_party packages (here, two github.com/ entries).

import (
    "chromiumos/tast/crash"
...
    "chromiumos/tast/testing"
    "context"
...
    "time"

    "github.com/godbus/dbus"
    "github.com/mafredri/cdp/devtool"
)

I.e., goimports would misunderstand "chromiumos/.*" packages as if these are standard lib packages, because chromiumos packages does not have "." in its entry.
cf) https://github.com/golang/tools/blob/master/imports/fix.go#L63

The second command splits the first group into two thanks to "--local chromiumos/", 1) standardlib and 2) packages starts with "chromiumos/" in this order. Becuase goimports preserves empty lines, so it does effectively no-op for the github.com group. The group order is also preserved, so github.com group is at the end.
Thus, I think your vim would produce;

import (
    "context"
...
    "time"

    "chromiumos/tast/crash"
...
    "chromiumos/tast/testing"

    "github.com/godbus/dbus"
    "github.com/mafredri/cdp/devtool"
)

TBH, that looks not a simple procedure... My recommendation is just following "goimports --local chromiumos/". Starting from non-empty-line separation, it would produce

import (
    "context
...
    "time"

    "github.com/godbus/dbus"
    "github.com/mafredri/cdp/devtool"

    "chromiumos/tast/crash"
...
    "chromiumos/tast/testing"
)

Note that goimports preserves empty lines and group order if already exist. So, just running "goimports --local chromiumos/" for the existing chrome.go wouldn't work as expected,
because it already has empty line separation.
Could you try to remove empty lines in import decl first, then run "goimports --local=chromiumos/"?
That is actually what I did for all tast and tast-tests .go files in https://crrev.com/c/1270538.

WDYT?

Thanks again! I think I found a way to get the desired behavior in Vim.

My old approach followed the goimports suggestion from http://g3doc/go/g3doc/vim and additionally included:

  let g:gofmt_command = "goimports -local chromiumos/"

Instead, I now created a small goimports_cros script and placed it in $PATH:

  #!/bin/sh
  goimports -local chromiumos/

I set the following in .vimrc instead of g:gofmt_command:

  Glug codefmt gofmt_executable="goimports_cros"

(I tried adding "$*" at the end of the command in the script, but it fails. I'm not sure why; I can't see the error.)

--

I guess everyone will need to remember to not insert any blank lines into their import lists (since goimports will preserve them), right?

Yes, to rely on goimports grouping.
I with if goimports had an option to ignore existing grouping...

Or, we may want to develop "goimports_cros" which;
- First removes empty lines in import(), then
- runs "goimports --local chromiumos/" compatible.
It should produce always the same format, IIUC.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/3794b57d0e6000252aeb74d53e053c669400e609

commit 3794b57d0e6000252aeb74d53e053c669400e609
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Wed Oct 10 12:19:42 2018

tast: Sort import entries.

For consistency, use "goimports --local=chromiumos/" style.

BUG= chromium:891300 
TEST=Ran try.

Change-Id: I98816b71cc32b5144d4614cd3dc5bd78508c3c09
Reviewed-on: https://chromium-review.googlesource.com/1270538
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/run_cmd_test.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/run/remote.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/main.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/run_cmd.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/run/local.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/run/local_test.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/list_cmd.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/run/remote_test.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/build_cmd.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/list_cmd_test.go
[modify] https://crrev.com/3794b57d0e6000252aeb74d53e053c669400e609/src/chromiumos/cmd/tast/symbolize_cmd.go

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/7ac21caa911a2811c1b14fbe987b8a7b50e16580

commit 7ac21caa911a2811c1b14fbe987b8a7b50e16580
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Tue Oct 16 17:30:50 2018

tast: Implement import order lint.

BUG= chromium:891300 
TEST=Ran run_lint.sh locally.

Change-Id: I0be548e1dc080f1eccc35550dc76a0c4ec1555ad
Reviewed-on: https://chromium-review.googlesource.com/1270537
Commit-Ready: Hidehiko Abe <hidehiko@chromium.org>
Tested-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[add] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/check/import_order.go
[add] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/check/import_order_test.go
[add] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/check/common_test.go
[modify] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/check/errors_test.go
[modify] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/check/issue.go
[modify] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/check/errors.go
[modify] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/docs/writing_tests.md
[modify] https://crrev.com/7ac21caa911a2811c1b14fbe987b8a7b50e16580/src/chromiumos/cmd/tast-lint/main.go