Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go/tools/bazel: refactor runfiles functionality #2076

Merged
merged 4 commits into from
Jun 3, 2019

Conversation

jayconrod
Copy link
Contributor

  • Refactored Runfile, FindBinary, and other functions. Everything now
    calls a single initialization function through sync.Once.
  • Added ListRunfiles. This will be used to re-create trees of
    runfiles, which will be needed for the new version of bazel_test.
  • Moved and rewrote tests in tests/core/runfiles.

Fixes #2031
Fixes #1728

* Refactored Runfile, FindBinary, and other functions. Everything now
  calls a single initialization function through sync.Once.
* Added ListRunfiles. This will be used to re-create trees of
  runfiles, which will be needed for the new version of bazel_test.
* Moved and rewrote tests in tests/core/runfiles.

Fixes bazel-contrib#2031
Fixes bazel-contrib#1728
@jayconrod jayconrod requested a review from ianthehat May 25, 2019 19:55
@jayconrod jayconrod merged commit 4442d82 into bazel-contrib:master Jun 3, 2019
@jayconrod jayconrod deleted the runfiles branch June 3, 2019 21:58
jayconrod added a commit that referenced this pull request Jul 8, 2019
* Refactored Runfile, FindBinary, and other functions. Everything now
  calls a single initialization function through sync.Once.
* Added ListRunfiles. This will be used to re-create trees of
  runfiles, which will be needed for the new version of bazel_test.
* Moved and rewrote tests in tests/core/runfiles.

Fixes #2031
Fixes #1728
jayconrod added a commit that referenced this pull request Jul 8, 2019
* Refactored Runfile, FindBinary, and other functions. Everything now
  calls a single initialization function through sync.Once.
* Added ListRunfiles. This will be used to re-create trees of
  runfiles, which will be needed for the new version of bazel_test.
* Moved and rewrote tests in tests/core/runfiles.

Fixes #2031
Fixes #1728
jayconrod added a commit that referenced this pull request Jul 8, 2019
* Refactored Runfile, FindBinary, and other functions. Everything now
  calls a single initialization function through sync.Once.
* Added ListRunfiles. This will be used to re-create trees of
  runfiles, which will be needed for the new version of bazel_test.
* Moved and rewrote tests in tests/core/runfiles.

Fixes #2031
Fixes #1728
@alexeagle
Copy link
Contributor

hey there, I think this change broke rules_typescript
https://github.com/bazelbuild/rules_typescript/blob/master/devserver/runfiles/runfiles.go
it seems that Runfile("build_bazel_rules_nodejs/packages/protractor/test/protractor-2/app.js") used to work (starts with workspace name) but now it must be Runfile("packages/protractor/test/protractor-2/app.js") (no workspace name)?
Does that sound right?

@jayconrod
Copy link
Contributor Author

Are you able to switch to not using the workspace name? Runfile should only accept relative paths from the workspace root, but it was (and is still) way too permissive on the input path.

I'm sorry for the break though. Runfile was originally written by a few external contributors, and it wasn't adequately documented or tested. I needed to refactor it and make it work on Windows to make progress, but it was hard to make any change without subtly breaking something.

@alexeagle
Copy link
Contributor

Is it really the right interface to accept a path with no workspace name? If I have a file index.js in the local workspace runfiles and also in an external one, I would have no way to locate the external index.js in the runfiles.

@jayconrod
Copy link
Contributor Author

It depends on the situation, but no, Runfile is a poor interface.

ListRunfiles provides the most complete information. I'd recommend using that if files may be in multiple workspaces or if you're using a mix of short paths and regular paths.

If you can pass the location directly through a build file with something args = ["$(location :label)"], that may let you avoid this library altogether.

@alexeagle
Copy link
Contributor

I'm not ready to figure out that refactoring right now so I'll just patch out this commit from our rules_go http_archive fetch. One day the merge conflict will bring someone back to this thread. Maybe me! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go/tools/bazel: FindBinary does not work on Windows Runfile in go/tools/bazel does not work with binaries
3 participants