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

Clean test run #829

Merged
merged 2 commits into from
Jan 18, 2023
Merged

Clean test run #829

merged 2 commits into from
Jan 18, 2023

Conversation

shs96c
Copy link
Collaborator

@shs96c shs96c commented Jan 15, 2023

With these changes, bazel test //... on macOS runs cleanly, without failures.

This test has been failing for a while, and yet we've never
noticed, and we don't seem to care. It depends on us exposing
the cached files from coursier. Given that this code is stable
and unlikely to change (see bazel-contrib#807)
this feels like a safe change to make.
@shs96c shs96c requested a review from jin January 15, 2023 11:23
@shs96c shs96c requested a review from cheister as a code owner January 15, 2023 11:23
String uriString = uri.toString();
if (uriString.contains("repo1.maven.org") && uriString.contains("guava")) {
foundGuavaJar = true;
if (OS.indexOf("mac") >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this test not pass on Mac? Or do we just not need this test anymore?

Copy link
Collaborator Author

@shs96c shs96c Jan 15, 2023

Choose a reason for hiding this comment

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

It doesn't pass anywhere. Ever since we switched to adding explicit visibilities on targets rather than having it set at the package level (#649), this test hasn't passed anywhere since we're not exporting the files. I've no idea why our CI runs haven't caught this, but it's been that way for a year now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's delete the feature, then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I can do that.

@@ -12,10 +12,22 @@ MavenBomFragmentInfo = provider(
)

def _maven_bom_fragment_impl(ctx):
java_info = ctx.attr.artifact[JavaInfo]

# Recent Bazel versions
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: mention the exact version cutover

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@shs96c shs96c merged commit 9f9ce4e into bazel-contrib:master Jan 18, 2023
@shs96c shs96c deleted the clean-test-run branch January 18, 2023 11:43
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.

3 participants