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 testonly attribute handling #955

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Fix testonly attribute handling #955

merged 1 commit into from
Sep 6, 2023

Conversation

meteorcloudy
Copy link
Member

@meteorcloudy meteorcloudy commented Sep 6, 2023

Fix the following error when using test_only in Bzlmod

ERROR: Traceback (most recent call last):
	File "/private/var/tmp/_bazel_pcloudy/829441223e9fec5a5a2e3d1dd743fdf0/external/rules_jvm_external~5.2/private/extensions/maven.bzl", line 304, column 52, in _maven_impl
		artifacts_json = [_json.write_artifact_spec(a) for a in artifacts]
	File "/private/var/tmp/_bazel_pcloudy/829441223e9fec5a5a2e3d1dd743fdf0/external/rules_jvm_external~5.2/specs.bzl", line 230, column 38, in _artifact_spec_to_json
		"\", \"version\": \"" + artifact_spec["version"] + "\""
Error: unsupported binary operation: string + bool

@meteorcloudy
Copy link
Member Author

Is there any way we can improve the quality of Bzlmod support and have more test coverage perhaps?

@meteorcloudy
Copy link
Member Author

Also I noticed "fetch_sources": attr.bool(default = True), in Bzlmod: https://github.com/bazelbuild/rules_jvm_external/blob/5.2/private/extensions/maven.bzl#L41

And eventually it's an OR of all maven installs. Is there any reason for this attribute to be True by default in Bzlmod?

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

Ooops. Nice catch. Thank you! LGTM

@shs96c shs96c merged commit 5509cf1 into master Sep 6, 2023
8 checks passed
@shs96c shs96c deleted the meteorcloudy-patch-1 branch September 6, 2023 18:58
@shs96c
Copy link
Collaborator

shs96c commented Sep 6, 2023

We set the default to be True because the common case is for people to use an IDE for working on their code, and having the sources available makes that experience significantly nicer. We can change it to False if necessary, and it's probably best if we take the value from the root module only.

@meteorcloudy
Copy link
Member Author

We can change it to False if necessary, and it's probably best if we take the value from the root module only.

Agree, in fact, currently it's not possible for the root module to turn this off if another dependency is using maven install without overriding it to False.

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.

None yet

2 participants