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

fix: dangerous_test_thirdparty_version.sh #1434

Closed
wants to merge 2 commits into from

Conversation

tanishiking
Copy link
Contributor

@tanishiking tanishiking commented Oct 12, 2022

Description

CONTRIBUTING.md says we should run ./dangerous_test_thirdparty_version.sh if we change in third-party, but the script fails with the following error.

Log:

sed: can't read : No such file or directory
Loading:
Loading: 0 packages loaded
ERROR: Traceback (most recent call last):
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/WORKSPACE", line 29, column 19, in <toplevel>
                scala_repositories(("2.12.10", { "scala_compiler": "cedc3b9c39d215a9a3ffc0cc75a1d784b51e9edc7f13051a1b4ad5ae22cfbc0c","scala_library": "0a57044d10895f8d3dd66ad4286891f607169d948845ac51e17b4c1cf0ab569d","scala_reflect": "56b609e1bab9144fb51525bfa01ccd72028154fc40a58685a1e9adcbe7835730" }))
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/scala/private/macros/scala_repositories.bzl", line 72, column 21, in scala_repositories
                repositories(
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/third_party/repositories/repositories.bzl", line 31, column 37, in repositories
                _scala_maven_import_external(
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/scala/scala_maven_import_external.bzl", line 240, column 30, in scala_maven_import_external
                jvm_maven_import_external(
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/scala/scala_maven_import_external.bzl", line 261, column 44, in jvm_maven_import_external
                jar_urls = _convert_coordinates_to_urls(coordinates, server_urls)
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/scala/scala_maven_import_external.bzl", line 171, column 46, in _convert_coordinates_to_urls
                urls.append(_concat_with_needed_slash(server_url, url_suffix))
        File "/Users/tanishiking/src/github.com/tanishiking/rules_scala/scala/scala_maven_import_external.bzl", line 175, column 18, in _concat_with_needed_slash
                if server_url.endswith("/"):
Error: 'dict' value has no field or method 'endswith'
ERROR: error loading package '': Encountered error while reading extension file 'proto/repositories.bzl': no such package '@rules_proto//proto': error loading package 'external': Could not load //external package
INFO: Elapsed time: 0.460s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
ERROR: Couldn't start the build. Unable to run tests
FAILED: Build did NOT complete successfully (0 packages loaded)
 Test "test_scala_version 2.12.10 cedc3b9c39d215a9a3ffc0cc75a1d784b51e9edc7f13051a1b4ad5ae22cfbc0c 0a57044d10895f8d3dd66ad4286891f607169d948845ac51e17b4c1cf0ab569d 56b609e1bab9144fb51525bfa01ccd72028154fc40a58685a1e9adcbe7835730" failed  (0 sec)

dangerous_test_thirdparty_version.sh modifies scala_repository in WORKSPACE file to switch scala version for testing bazel test //third_party/...,

However, this logic is implemented when scala_config(scala_version=...) was not yet available.

this commit changes the test to use scala_version to switch scala_version, which fix the test.


However, there're still some issues with this script

  • It takes quite a long time to run tests for all Scala versions
  • The list of Scala versions are bit old
  • This test is still failing for example for 2.12.10
    • (while it passes for default scala version, verified by bazel test //third_party/... running on CI (via test_rules_scala.sh))

Maybe we should update scala_versions in this script, and run only some latest scala versions? Or, should we maintain this test script any longer?

Motivation

#1433
CONTRIBUTING.md suggested running dangerous_test_thirdparty_version.sh when we change third_party code, and I found this test is broken.

This test modifies `scala_repository` in `WORKSPACE` file to
switch scala version for testing `bazel test //third_party/...`,

However, this logic is implemented when scala_config(scala_version=...)
was not yet available.

- test implemented at the beginning of 2020 bazelbuild#971
- scala_version is implemented at the end of 2020
  - c439ebf

this commit changes the test to use `scala_version` to switch
scala_version, that fix the test.
Copy link
Collaborator

@liucijus liucijus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for looking into this @tanishiking!
I think we can drop tests for versions that are older than we have in repositories

@tanishiking
Copy link
Contributor Author

tanishiking commented Oct 12, 2022

I think we can drop tests for versions that are older than we have in repositories

👍 I'll update the script later :)

edit: updated a908088

@tanishiking
Copy link
Contributor Author

Looks like the test failed for some network reason (maybe), re-run it may fix

Copy link
Collaborator

@liucijus liucijus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately overrides still need to be passed for non default Scala versions

"14a520328ea4ca7f423b30154a54d3df0a531a9c51f5e98eda272c9821bc5331" \
"fd896db4806875f538843ea24411e483ee4d0734710a108d0308ef108e83cf80"
$runner test_scala_version "2.13.6"
$runner test_scala_version "2.13.7"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overrides are still needed for versions not defined in default Rules Scala repositories. scala_config only selects repository by it's major version, so in case 2.13.7, still 2.13.6 is used as defined in https://github.com/bazelbuild/rules_scala/blob/master/third_party/repositories/scala_2_13.bzl#L3. Unfortunately there is no good way to check the overridden version and report it to the user.

@liucijus
Copy link
Collaborator

closing as stale, feel free to reopen

@liucijus liucijus closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants