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

Bazel aspects: allow sharing source jars and allow empty sourcejars #757

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

antonsviridov-src
Copy link
Contributor

@antonsviridov-src antonsviridov-src commented Oct 30, 2024

Closes GRAPH-948
Closes GRAPH-949

Test plan

  • Reproducers added to existing cases

@antonsviridov-src antonsviridov-src changed the title Use labelname when declaring output for unzipped sourcejar Bazel aspects: allow sharing source jars and allow empty sourcejars Oct 30, 2024
@antonsviridov-src antonsviridov-src marked this pull request as ready for review October 30, 2024 15:06
output_dir.append(dir)

ctx.actions.run_shell(
inputs = javac_action.inputs,
outputs = [dir],
mnemonic = "ExtractSourceJars",
command = """
unzip {input_file} -d {output_dir}
[ "$(unzip -q -l {input_file} | wc -l)" -eq 0 ] || unzip {input_file} -d {output_dir}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inexplicably, unzip returns a 1 exit code if there are any warnings (see Diagnostics on https://linux.die.net/man/1/unzip) with no way to turn it off.

So we just literally count the number of files..

@antonsviridov-src
Copy link
Contributor Author

antonsviridov-src commented Oct 31, 2024

@JohnnyMorganz would you like to build this PR from source and try on your repos?
Alternatively if you know of any sizeable public bazel repo that uses srcjars, I could use it for testing instead.

Copy link

@kritzcreek kritzcreek left a comment

Choose a reason for hiding this comment

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

If it works, it works. As discussed yesterday :)

Ideally we'd wait with merging until getting positive feedback that it fixes the issue for the customer.

@JohnnyMorganz
Copy link

Thanks for checking in!

Looking good from our side, but I am seeing one issue with test targets. You can repro this by changing the java_library to a java_test:

java_test(
    name = "testing",
    srcs = [
        "Foo.java",
        ":generated-srcjar",
    ],
    test_class = "Foo",
)

Running the indexing gives the following error:

$ /home/code/scip-java/out/bin/scip-java index --build-tool=bazel --bazel-scip-java-binary="/home/code/scip-java/out/bin/scip-java" --output=index-java.scip -- -- //test/
ERROR: One of the output paths 'bazel-out/k8-dbg--cd/bin/test/testing' (belonging to //test:testing) and 'bazel-out/k8-dbg--cd/bin/test/testing/extracted_srcjar/test/sources.srcjar' (belonging to //test:testing) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them
ERROR: com.google.devtools.build.lib.actions.ArtifactPrefixConflictException: One of the output paths 'bazel-out/k8-dbg--cd/bin/test/testing' (belonging to //test:testing) and 'bazel-out/k8-dbg--cd/bin/test/testing/extracted_srcjar/test/sources.srcjar' (belonging to //test:testing) is a prefix of the other. These actions cannot be simultaneously present; please rename one of the output files or build just one of them

Sorry, unfortunately I do not know of any good public repo using srcjars to test with

@antonsviridov-src antonsviridov-src merged commit c3ab8ea into main Nov 6, 2024
24 checks passed
@antonsviridov-src antonsviridov-src deleted the bazel-aspect-fix-shared-srcjar branch November 6, 2024 12:05
@antonsviridov-src
Copy link
Contributor Author

Thanks @JohnnyMorganz, I'll try to address in a separate PR.

Will rely on your reproductions then :)

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