"buck project" to copy generated Go code to vendor #1927

linzhp · 2018-06-18T22:27:53Z

For most Go IDE/tools to work, every imported package has to be in some proper directories under GOPATH. However, Go code generated during a Buck build is not. This PR copies them to the vendor directory as part of "buck project --ide=intellij", although this change not only makes the IntelliJ/GoLand work, but also enable other IDEs and static check tools work with Go projects built with Buck.

In order for "buck project" command to identify Go targets and IjProjectCommandHelper to use Go related classes, I moved the Go module from features to core.

@styurin @kageiit Please review

styurin · 2018-06-18T22:31:46Z

Let's add support for modules in project command. Moving Go back to core would be a regression.

I think we can do that by adding a plugin extension point that would provide implementations of project command. No need to move existing project commands to this model. I can take a look at this, but it'll take me some time to do that.

linzhp · 2018-06-18T23:01:01Z

This is currently a blocking issue for us. As a compromise, can we keep Go in the core temporarily, and we are committed to move it back to features once you have that plugin extension point ready? JVM and Apple languages are still in the core anyway, and that is why project command is able to create IntelliJ and XCode projects.

We don't like the idea of keeping a private fork for this PR, as it moves many files, and changes from upstream are likely to result in a painful conflict and merge process. It's relatively easier to Go out of core.

linzhp · 2018-06-19T06:10:10Z

Figured out a way to load Go extension in runtime. So now Go module is out of core again. Please take another look @styurin

vitarb · 2018-06-19T06:31:12Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

@@ -233,6 +251,75 @@ private TargetGraph getProjectGraphForIde(
        buckEventBus, cell, enableParserProfiling, executor, passedInTargets);
  }

+  @SuppressWarnings("unchecked")
+  private ExitCode copyGeneratedGoSource(TargetGraphAndTargets targetGraphAndTargets)


Can you please add some comments on how it works and what assumptions we make.

vitarb · 2018-06-19T06:31:47Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

+  @SuppressWarnings("unchecked")
+  private ExitCode copyGeneratedGoSource(TargetGraphAndTargets targetGraphAndTargets)
+      throws IOException, InterruptedException {
+    HashMap<String, BuildTargetSourcePath> generatedPackages = new HashMap<>();


s/HashMap/Map in the declaration

vitarb · 2018-06-19T06:33:03Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

+                    .collect(
+                        Collectors.toMap(
+                            src -> packageName.get(), src -> (BuildTargetSourcePath) src)));
+          }


else fail the build? Are there any valid cases when go library may have no package name?

Yes there are, but we don't. Figuring out a proper default package name will need to load another class from go module and use the same reflection trick to call its methods.

vitarb · 2018-06-19T06:34:55Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

+          Optional<String> packageName =
+              (Optional<String>) getPackageNameMethod.invoke(constructorArg);
+          if (packageName.isPresent()) {
+            generatedPackages.putAll(


Should we remove some line breaks from these chaining calls? (same relates to some other statements below)

Those line breaks are added by Google Java formatter, in order to pass CI checks

vitarb · 2018-06-21T22:47:00Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

@@ -304,6 +304,7 @@ private void copyGeneratedGoCode(
        DefaultSourcePathResolver.from(new SourcePathRuleFinder(result.getActionGraphBuilder()));
    for (BuildTargetSourcePath sourcePath : generatedPackages.keySet()) {
      Path desiredPath = vendorPath.resolve(generatedPackages.get(sourcePath));
+      fs.deleteRecursivelyIfExists(desiredPath);


This will not handle sub-packages well.
For example if we have:
github.com/foo/bar
github.com/foo/bar/A
and process the later first then contents of A will be removed while processing root package of the repo.

Let's make sure we cleanup only contents of the current package.

linzhp · 2018-06-28T22:58:02Z

Pinging @styurin @ttsugriy @philipjameson Could you review this PR?

linzhp · 2018-07-05T21:32:53Z

Pinging @styurin @ttsugriy @philipjameson again

facebook-github-bot

@mkaczanowski has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

styurin · 2018-07-06T16:35:32Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

@@ -190,7 +206,10 @@ public ExitCode parseTargetsAndRunProjectGenerator(List<String> arguments)

      return ExitCode.SUCCESS;
    }
-
+    ExitCode result = initGoWorkspace(targetGraphAndTargets);


So it will affect all the invocations of buck project even if it's not related to Go?

No, initGoWorkspace only scans go_library rules. If there is no such rule in the workspace, it is a no-op.

From the way it looks it should implemented as a separate command.

For which IDE this command is implemented? Is that just copying files?

This has been tested for GoLand and Visual Studio Code. Yes, it is just copying files. This is sufficient for these IDEs to work. Go is more like "convention over configuration" style. So it has strong conventions about where packages should be put, rather than relying on configuration files to tell the IDEs and Go toolchain where to search for packages.

styurin · 2018-07-06T16:35:44Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

+    Path vendorPath;
+    ProjectFilesystem fs = cell.getFilesystem();
+    if (fs.exists(Paths.get("src"))) {
+      vendorPath = Paths.get("src", "vendor");


What's vendor?

vendor is a place to put third-party libraries. Go toolchain and IDEs is able to automatically pick up the Go packages in vendor directory. It is a good place to put Go packages that we don't want to put into source control.

We do not have repository-specific details in Buck logic. You need to change it to smarter about detecting this location.

It's not really repository-specific. Vendor folder is a golang concept: https://golang.org/cmd/go/#hdr-Vendor_Directories

Note that in contrast to java Go currently doesn't have ability to link sources/jars from random locations. There is a WIP workspaces feature on Google's side that should enable better integration of go toolchain with build tools like buck or bazel but it's not released yet and final ETA is not clear.
Currently we are in the situation when all generated or downloaded code would not be discoverable in the IDE. Putting it in vendor allows us to easily browse code and be able to run third-party tools not integrated with buck that are expecting valid GOPATH folder with all dependencies in place. Although can probably be more generic, with this change we are going from the state where buck project is barely useful for go to the state where it sets up a project with valid dependencies.

Vendor directory can be anywhere under GOPATH/src, and Go toolchain automatically detects that. if (fs.exists(Paths.get("src"))) is just in case buck root is at GOPATH, i.e., the parent of src directory. Of course, this logic would fail if buck root is in a higher directory than GOPATH. In that case, Buck has to search all directories under buck root for src. It is expensive and non-deterministic (e.g., what if there are more than one src?). We don't want to sacrifice the performance and make the logic more complicated to handle a scenario that doesn't exist for us. We will leave it for future users when they have a real use case.

We don't really want to change Buck when there is a change in where these libs are located. You can put it in a configuration option and set it to whatever you need in your repository.

Go toolchain and IDEs don't take configurations like that, unfortunately. Only packages in the main src directory and vendor directories are recognized.

Go toolchain and IDEs don't take configurations like that

But what if they change location? You would need to change this code to make it working, then deal with support of the different versions of these tools.

A configuration option makes it much easier to maintain. You can even use a configuration option with default value of src/vendor when it's not present.

The approach here is very flexible, not repository specific at all. If a project has its own vendor folder at GOPATH/src/foo/bar/vendor, and the buck root is at GOPATH, then the generated code will be copied into GOPATH/src/vendor. If the buck root is at GOPATH/src/foo, the the generated code will be copied to GOPATH/src/foo/vendor. All of these vendor folders are recognized by Go toolchain and IDEs at the same time.

Of course, if you want the vendor folder location to be further configurable by users, I can do that too.

Overall, I see this PR a temporary solution to unblock Go developers using Buck. For long term solution, there are discussions on Bazel side for over a year (1, 2). We may want to watch with Bazel community and see what they end up with. Whatever the long term solution ends up, I am happy to assist the migration.

If you have further concerns about this feature, I can also add a configuration that disable this feature by default.

I also notice that @mkaczanowski recently commit changes trying to support IDEs (great thanks to @mkaczanowski), but the performance is somehow quite bad (I need to dig a bit deeper to understand why), and I am not quite sure about symlink tree too, as people in Bazel run into problems with symlink trees (3)

styurin · 2018-07-06T16:36:19Z

src/com/facebook/buck/ide/intellij/IjProjectCommandHelper.java

+      if (classLoader != null) {
+        goLibraryDescriptionArgClass =
+            Class.forName(
+                "com.facebook.buck.features.go.GoLibraryDescriptionArg", true, classLoader);


Could you wait with that? I'll try to migrate project command to a plugin model, but it can take some time.

Do you see any difficulties with changing this in future?

We really need this feature now. Since changes in this PR is very localized, it is easy to change whenever you migrate project command. I am happy to assist you for migrating this piece of code.

linzhp · 2018-07-09T19:01:21Z

@mkaczanowski You imported this PR 4 days ago. Could you check why it is still not merged yet?

Purge existing vendor package before copying genrated code

facebook-github-bot · 2018-07-13T22:40:11Z

@linzhp has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-07-13T22:51:19Z

@linzhp has updated the pull request. Re-import the pull request

facebook-github-bot

@styurin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mkaczanowski · 2018-07-17T11:46:03Z

thanks @linzhp this is very useful for us as well!

facebook-github-bot added the CLA Signed label Jun 18, 2018

styurin added the enhancement label Jun 18, 2018

vitarb reviewed Jun 19, 2018

View reviewed changes

kageiit approved these changes Jun 19, 2018

View reviewed changes

vitarb reviewed Jun 21, 2018

View reviewed changes

facebook-github-bot reviewed Jul 6, 2018

View reviewed changes

styurin suggested changes Jul 6, 2018

View reviewed changes

mkaczanowski added the golang label Jul 9, 2018

linzhp added 3 commits July 13, 2018 13:49

"buck project" to copy generated Go code to vendor

707675c

Purge existing vendor package before copying genrated code

Fixed merge errors

1f26092

Move go project test

4b0dc69

linzhp force-pushed the gen_src branch from ce75fcd to 4b0dc69 Compare July 13, 2018 22:40

Use go.vendor_path for generated code

e41d33e

facebook-github-bot reviewed Jul 13, 2018

View reviewed changes

facebook-github-bot added the Import Started label Jul 14, 2018

facebook-github-bot closed this in 7a54a36 Jul 14, 2018

linzhp deleted the gen_src branch July 20, 2018 04:04

"buck project" to copy generated Go code to vendor #1927

"buck project" to copy generated Go code to vendor #1927

Conversation

linzhp commented Jun 18, 2018

styurin commented Jun 18, 2018

linzhp commented Jun 18, 2018

linzhp commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linzhp commented Jun 28, 2018

linzhp commented Jul 5, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linzhp Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linzhp commented Jul 9, 2018

facebook-github-bot commented Jul 13, 2018

facebook-github-bot commented Jul 13, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

mkaczanowski commented Jul 17, 2018

linzhp Jul 13, 2018 •

edited

Loading