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.

Sign up for GitHub

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

Jump to bottom

Add Bazel support, fixes #88 #439

Merged
merged 6 commits into from
May 10, 2022
Merged

Add Bazel support, fixes #88 #439

merged 6 commits into from
May 10, 2022
+1,398 −47

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented May 5, 2022 •
edited
Loading

This PR adds a new Bazel build to this codebase so that Bazel users can
build the semanticdb-javac compiler plugin from source and use it in
their Bazel build. This PR does not yet include documentation on how to use this configuration,
but this PR adds an end-to-end example of how to import the lsif-java rules via include(...)

Test plan

See new "bazel" CI job.

@olafurpg olafurpg force-pushed the bazel-build branch 3 times, most recently from 3deacd9 to 4bb14ad Compare May 5, 2022 10:59
@olafurpg olafurpg requested a review from varungandhi-src May 5, 2022 10:59
@olafurpg
Improve support for Bazel builds, towards #88

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
beda410 
This commit adds a new Bazel build to this codebase so that Bazel users can
build the semanticdb-javac compiler plugin from source and use it in
their Bazel build. This commit includes examples of how a Bazel user
could potentially enable the compiler plugin dynamically based on a
command-line flag, but we don't know if this is the best way to
integrate lsif-java with a Bazel codebase.

This commit is just an incremental step towards Bazel support, it's not
a complete solution yet because the `lsif-java index-semanticdb` command
still needs additional functionality to read jar files from the
`bazel-bin` directory.
@varungandhi-src
Copy link
Contributor

varungandhi-src commented May 5, 2022 •
edited
Loading

Manually test the Bazel build. We don't yet compile the Bazel build in CI.

How hard would it be to add this? Any specific reason to do it later vs now?

varungandhi-src
varungandhi-src reviewed May 5, 2022
View reviewed changes
@olafurpg
Copy link
Member Author

olafurpg commented May 5, 2022

I'm working on expanding this PR to include full support. We can keep this PR open until all the pieces are ready.

varungandhi-src
varungandhi-src reviewed May 5, 2022
View reviewed changes
varungandhi-src
varungandhi-src reviewed May 5, 2022
View reviewed changes
semanticdb-javac/BUILD
Comment on lines +8 to +17
config_setting(
name = "is_enabled",
flag_values = {":enabled": "true"},
)

string_flag(
name = "enabled",
values = ["true", "false"],
build_setting_default = "false",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for having both is_enabled and enabled? 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

One of them defines the name when specifying the flag from the command-line bazel build //... //semanticdb-javac:enabled=true.

The is_enabled target is used as a filter for a select(...) clause.

See official docs https://bazel.build/docs/configurable-attributes

varungandhi-src
varungandhi-src reviewed May 5, 2022
View reviewed changes
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbTaskListener.java
Comment on lines +150 to +160
int relativePathDepth = 0;
int uriPathDepth = uriPath.getNameCount();
int absolutePathDepth = absolutePath.getNameCount();
while (relativePathDepth < uriPathDepth && relativePathDepth < absolutePathDepth) {
String uriName = uriPath.getName(uriPathDepth - relativePathDepth - 1).toString();
String pathName = absolutePath.getName(absolutePathDepth - relativePathDepth - 1).toString();
if (!uriName.equals(pathName)) {
break;
}
relativePathDepth++;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion(non-blocking): Perhaps add some unit tests for this function or specifically this logic? It seems a bit tricky.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I opened a reminder issue and a // FIXME https://github.com/sourcegraph/lsif-java/issues/444 comment. Don't want to delay this PR further.

@olafurpg
Complete Bazel support.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
1f30836 
- Add LSIF generation support.
- Add e2e example on how to import lsif-java as an external repo.
- Add cross-repo support for bazelbuild/rules_jvm_external and twitter/bazel-multiversion.
olafurpg
olafurpg commented May 9, 2022
View reviewed changes
Copy link
Member Author

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you @varungandhi-src for the detailed review! I appreciate the thoughtful comments and the resulting code is much cleaner.

semanticdb-javac/BUILD
Comment on lines +8 to +17
config_setting(
name = "is_enabled",
flag_values = {":enabled": "true"},
)

string_flag(
name = "enabled",
values = ["true", "false"],
build_setting_default = "false",
)
Copy link
Member Author

Choose a reason for hiding this comment

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

One of them defines the name when specifying the flag from the command-line bazel build //... //semanticdb-javac:enabled=true.

The is_enabled target is used as a filter for a select(...) clause.

See official docs https://bazel.build/docs/configurable-attributes

semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbTaskListener.java
Comment on lines +150 to +160
int relativePathDepth = 0;
int uriPathDepth = uriPath.getNameCount();
int absolutePathDepth = absolutePath.getNameCount();
while (relativePathDepth < uriPathDepth && relativePathDepth < absolutePathDepth) {
String uriName = uriPath.getName(uriPathDepth - relativePathDepth - 1).toString();
String pathName = absolutePath.getName(absolutePathDepth - relativePathDepth - 1).toString();
if (!uriName.equals(pathName)) {
break;
}
relativePathDepth++;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I opened a reminder issue and a // FIXME https://github.com/sourcegraph/lsif-java/issues/444 comment. Don't want to delay this PR further.

@olafurpg
Address review comments

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
42b1f11
@olafurpg
Copy link
Member Author

olafurpg commented May 9, 2022

I updated a new "bazel" CI job to test the generation e2e for internal usage (within our repo) and external usage (how end-users will consume lsif-java). Once we have snapshot testing logic we can additionally snapshot the generated index files, right now we just test that dump.lsif-typed gets created.

@olafurpg
Remove dependency pinning for Maven

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
f7bf94d
@olafurpg olafurpg changed the title Improve support for Bazel builds, towards #88 Add Bazel support, fixes #88 May 9, 2022
@olafurpg
Copy link
Member Author

olafurpg commented May 9, 2022

The CI tests are s now green in CI! I didn't get dependency pinning working but I don't think it's a problem because we have so few dependencies and they have no conflicts so the resolution will always be the same.

varungandhi-src
varungandhi-src reviewed May 9, 2022
View reviewed changes
.github/workflows/ci.yml Outdated
Comment on lines 30 to 34
- run: bazel run lsif-semanticdb:bazel -- --sourceroot $PWD
- run: du -h dump.lsif-typed
- run: bazel build //... --@lsif_java//semanticdb-javac:enabled=true
working-directory: examples/bazel-example
- run: bazel run @lsif_java//lsif-semanticdb:bazel -- --sourceroot $PWD
Copy link
Contributor

@varungandhi-src varungandhi-src May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- run: bazel run lsif-semanticdb:bazel -- --sourceroot $PWD
- run: du -h dump.lsif-typed
- run: bazel build //... --@lsif_java//semanticdb-javac:enabled=true
working-directory: examples/bazel-example
- run: bazel run @lsif_java//lsif-semanticdb:bazel -- --sourceroot $PWD
- run: bazel run lsif-semanticdb:bazel -- --sourceroot "$PWD"
- run: du -h dump.lsif-typed
- run: bazel build //... --@lsif_java//semanticdb-javac:enabled=true
working-directory: examples/bazel-example
- run: bazel run @lsif_java//lsif-semanticdb:bazel -- --sourceroot "$PWD"

In case someone copy-pastes this to run locally, it'll not work if PWD has spaces.

varungandhi-src
varungandhi-src approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

LGTM, except for a couple of pending minor comments.

@olafurpg olafurpg merged commit e0fbe11 into main May 10, 2022
@olafurpg olafurpg deleted the bazel-build branch May 10, 2022 08:12
@olafurpg olafurpg mentioned this pull request May 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varungandhi-src varungandhi-src

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@olafurpg @varungandhi-src