-
Notifications
You must be signed in to change notification settings - Fork 275
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
Fix toolchain registration call for ScalaTest #1130
Fix toolchain registration call for ScalaTest #1130
Conversation
How come it passed until now? |
In |
Do you think it's a good idea to keep it like this? Feels like a recipe for future bugs |
@ittaiz I agree it's an opportunity for bugs. Added an example workspace which serves as an integration test for ScalaTest repository/toolchain helpers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test workspace! Looks really good :) left few questions
@@ -119,8 +128,11 @@ format_repositories() | |||
|
|||
http_archive( | |||
name = "io_bazel_rules_go", | |||
sha256 = "45409e6c4f748baa9e05f8f6ab6efaa05739aa064e3ab94e5a1a09849c51806a", | |||
url = "https://github.com/bazelbuild/rules_go/releases/download/0.18.7/rules_go-0.18.7.tar.gz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you upgrade rules_go? Because of skylib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
@@ -3,4 +3,4 @@ def scalatest_repositories(): | |||
pass | |||
|
|||
def scalatest_toolchain(): | |||
native.register_toolchain("//testing:scalatest_toolchain") | |||
native.register_toolchains("@io_bazel_rules_scala//testing:scalatest_toolchain") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question- your previous CI (IIRC) was without @io_bazel_rules_scala
right? Why the change? What pushed it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a mistake - all toolchains have to be registered with external name otherwise they may be treated as different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's precaution rather than something actually failed? (Ok with either, trying to understand which)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with the example repo - it doesn't even find the toolchain:
ERROR: Analysis of target '//example:example' failed; build aborted: invalid registered toolchain '//testing:scalatest_toolchain': no such package 'testing': BUILD file not found in any of the following directories. Add a BUILD file to a directory to mark it as a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing. Great that we have this test workspace
* Fix toolchain registration call for ScalaTest * Use external name in toolchain registration * Add example/integration test for ScalaTest repositories * Use the same skylib version and download it from google mirror * Update rules_go to support 1.0.3 skylib
Call correct function to register toolchain