Make DataFile work with Bazel, too. #87

abergmeier · 2018-03-18T23:51:29Z

DataFile claimed to do that before but did not work!
Now has fallback, which reads TEST_SRCDIR (set by Bazel) and builds
Bazely path. For this to work had to remove hardcoded Go paths from
DataFile calls.

alandonovan · 2018-03-19T01:41:33Z

eval_test.go

@@ -92,7 +92,7 @@ func TestEvalExpr(t *testing.T) {
 }

 func TestExecFile(t *testing.T) {
-	testdata := skylarktest.DataFile("skylark", ".")
+	testdata := skylarktest.DataFile("", ".")


Please don't remove the "skylark" component of the package name each call. Just because something is common to every call site does not mean it is the callee's responsibility to do it.

My reasoning is not that it is common.
The reasoning is that these arguments are a lie (see DataFile signature). It is not the package name as the callee suggests but a subpackage name at best. Overall DataFile builds a special constructed directory structure, which exclusively works with go build.
It does not work with Bazel and might not work for Buck, Gradle or SCons.

I see. I was tempted to suggest that the Bazel DataFile concatenate "com_github_google_"+subdir, but what you have seems fine.

alandonovan · 2018-03-19T01:44:29Z

skylarktest/skylarktest.go

 	"path/filepath"
 	"regexp"
 	"sync"

 	"github.com/google/skylark"
+	"github.com/google/skylark/bazeltest"


The purpose of making DataFile a variable containing a function is that you can override it during initialization of your application. There's no need for an "upwards" dependency from this package to bazeltest. Instead, get your bazel build file to compile this additional file into skylarktest:

package skylarktest func init() { DataFile = func(pkgdir, filename string) string { return filepath.Join("your/bazel/dir", pkgdir, filename) } }

An afterthought: why not move the bazeltest file into skylarktest package and tag that one file with //+bazel, and in your Bazel go_library rule for the skylarktest package, set that tag?

alandonovan · 2018-03-19T01:52:09Z

skylarktest/skylarktest.go

 	"path/filepath"
 	"regexp"
 	"sync"

 	"github.com/google/skylark"
+	"github.com/google/skylark/bazeltest"


An afterthought: why not move the bazeltest file into skylarktest package and tag that one file with //+bazel, and in your Bazel go_library rule for the skylarktest package, set that tag?

alandonovan · 2018-03-19T22:05:02Z

skylarktest/skylarktest.go

-// 'go build', under which a test runs in its package directory,
-// and Blaze, under which a test runs in the root of the tree.
-var DataFile = func(pkgdir, filename string) string {
+// test data resource. The symbol allows for overriding of the


Let's keep the previous comment and just change Blaze to "Bazel and Blaze".
The additional sentence is fine, but can be shortened to:

"The default behavior works for go build."

Let's keep the previous comment

Maybe it is me not being a native speaker but IMO the previous comment is plain wrong. The function does not abstract differences [...] but abstracts one specific way of building the file path (for go build). The assignable symbol then enables changing that behavior.

You're right that each anonymous function abstracts only one specific way, but think of it from the point of view of the client: DataFile, although it is technically a variable containing one of three functions, acts like a single function that hides (abstracts) each build system's peculiar way of locating data.

Fair enough. Still I try to make it a bit more descriptive.

alandonovan · 2018-03-19T22:05:46Z

skylarktest/skylarktest.go

+// function used to build file paths. Default bound function
+// processes go build paths.
+var DataFile = func(relDir, filename string) string {
+	pkgdir := "skylark/" + relDir


Let's fold pkgdir in to the next line, and rename relDir to subdir:

return filepath.Join(build.Default.GOPATH, "src/github.com/google/skylark", subdir, filename)

FWIW, the implementation we use under Blaze looks like:

return filepath.Join(runfiles.Path("google3/third_party/golang"), pkgdir, filename)

which means all three implementations apply filepath.Join to the two arguments, so in hindsight only one would have sufficed.

Let's fold pkgdir in to the next line, and rename relDir to subdir:

Will do. Side note is that this only works for a naive Bazel usage. There are at least 2 permutations of Bazel/rules_go flags, which are very likely to break even my current Bazel support.

alandonovan · 2018-03-19T22:06:10Z

skylarktest/bazeltest.go

+
+func init() {
+	testDir := os.Getenv("TEST_SRCDIR")
+	if len(testDir) == 0 {


testDir == ""

alandonovan · 2018-03-19T22:07:18Z

skylarktest/bazeltest.go

+		return
+	}
+
+	// Seems like we are running inside of Bazel


No, Bazel or Blaze. That's why I suggest you tag this file with //+bazel, and set that tag in the Bazel go_library rule, as Blaze does not want this behavior.

That's why I suggest you tag this file with //+bazel, and set that tag in the Bazel go_library rule, as Blaze does not want this behavior.

Sounds reasonable. You did lose me there however. It seems like you rely on some Golang (tag) magic. Never seen this before and my Google foo does not bring up anything useful. Can you please post a link to some documentation (hoping that for once there is a documentation for a Golang feature).

That's why I suggest you tag this file with //+bazel

Ah that is not a Golang but rather go build mechanics. Added that.

and set that tag in the Bazel go_library

AFAIK Bazel does not care about go build shenanigans so no tags to add. Works just fine ass is.

alandonovan · 2018-03-19T22:08:37Z

skylarktest/bazeltest.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Ugly override hack, which redefines DataFile if we assume to run inside


The solution is no uglier than the problem, so strike the first three words.

The solution is no uglier than the problem, so strike the first three words.

I do disagree since mechanics that work by convention creep me out. Then again, I can live without my three words of cheekiness :)

alandonovan · 2018-03-19T22:10:29Z

eval_test.go

@@ -92,7 +92,7 @@ func TestEvalExpr(t *testing.T) {
 }

 func TestExecFile(t *testing.T) {
-	testdata := skylarktest.DataFile("skylark", ".")
+	testdata := skylarktest.DataFile("", ".")


I see. I was tempted to suggest that the Bazel DataFile concatenate "com_github_google_"+subdir, but what you have seems fine.

Use new file skylarktest/bazeltest.go for handling Bazel. - Update comment for DataFile to something more helpful. - DataFile can now be replaced with Bazel variant

alandonovan reviewed Mar 19, 2018

View reviewed changes

abergmeier force-pushed the bazel_data_file branch 2 times, most recently from 2e9616a to 87d6271 Compare March 19, 2018 20:11

alandonovan reviewed Mar 19, 2018

View reviewed changes

abergmeier force-pushed the bazel_data_file branch from 87d6271 to 91ebe74 Compare March 20, 2018 08:15

Make DataFile work with Bazel, too.

afe8d2c

Use new file skylarktest/bazeltest.go for handling Bazel. - Update comment for DataFile to something more helpful. - DataFile can now be replaced with Bazel variant

abergmeier force-pushed the bazel_data_file branch from 91ebe74 to afe8d2c Compare March 20, 2018 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DataFile work with Bazel, too. #87

Make DataFile work with Bazel, too. #87

abergmeier commented Mar 18, 2018

alandonovan Mar 19, 2018

abergmeier Mar 19, 2018

alandonovan Mar 19, 2018

alandonovan Mar 19, 2018

alandonovan Mar 19, 2018

alandonovan Mar 19, 2018

alandonovan Mar 19, 2018

abergmeier Mar 19, 2018

alandonovan Mar 19, 2018

abergmeier Mar 20, 2018

alandonovan Mar 19, 2018

abergmeier Mar 19, 2018 •

edited

Loading

alandonovan Mar 19, 2018

alandonovan Mar 19, 2018

abergmeier Mar 19, 2018

abergmeier Mar 20, 2018

alandonovan Mar 19, 2018

abergmeier Mar 19, 2018

alandonovan Mar 19, 2018

Make DataFile work with Bazel, too. #87

Are you sure you want to change the base?

Make DataFile work with Bazel, too. #87

Conversation

abergmeier commented Mar 18, 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

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

abergmeier Mar 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abergmeier Mar 19, 2018 •

edited

Loading