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

Add new contribution point that allows other extension to register the build file pattern. #1845

Merged
merged 7 commits into from
Apr 20, 2021

Conversation

LeonovecSergey
Copy link
Contributor

Add new contribution point that allows other extension to register the build file pattern.

Close #1825

Signed-off-by: Siarhei Leanavets siarhei_leanavets1@epam.com

…e build file pattern.

Close redhat-developer#1825
Signed-off-by: Siarhei Leanavets siarhei_leanavets1@epam.com

Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
@jdneo
Copy link
Collaborator

jdneo commented Mar 25, 2021

Hi @LeonovecSergey,

I have two general comments about this PR.

  1. isBuildFilePattern() is called every time isJavaConfigFile() is called. At least we can cache all the available patterns to avoid iterating all the extension metadata every time.
  2. The pattern here should be defined more clearly, from the implementation it's a suffix. Maybe there is more space for improvement here. VS Code uses regular expression for its when clause. Can we also use that for our patterns?

Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
src/plugin.ts Outdated Show resolved Hide resolved
src/standardLanguageClient.ts Outdated Show resolved Hide resolved
    Use regular expression to check java build file names.

    Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>

Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
src/plugin.ts Outdated Show resolved Hide resolved
Bug fix: Extension change message is not displayed
Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
src/standardLanguageClient.ts Outdated Show resolved Hide resolved
src/plugin.ts Outdated Show resolved Hide resolved
@jdneo
Copy link
Collaborator

jdneo commented Apr 7, 2021

Have no idea why CI fails. BTW, @LeonovecSergey do you know how we can verify this change manually(like the steps using the Bazel extension to play around)?

@LeonovecSergey
Copy link
Contributor Author

Have no idea why CI fails. BTW, @LeonovecSergey do you know how we can verify this change manually(like the steps using the Bazel extension to play around)?

I use debug breakpoints to track java extension and build file patterns changes

  1. Open vscode-java fork in VSCode
  2. Launch as Extension
  3. In the opened window open some maven or gradle project (e.g. https://github.com/gradle/gradle-build-scan-quickstart)
  4. Update project (e.g shift+alt+U) for watching current state of java extensions and build file patterns
  5. Install new VSCode Extension that will add java extension or build file patterns from contributes in package.json (You can find vsix file in attached zip document)
  6. Click on the pop-up Reload button
  7. Update project (e.g shift+alt+U) for watching current state of java extensions and build file patterns
    bazel-ls-vscode-0.0.1.zip

Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
src/plugin.ts Outdated Show resolved Hide resolved
@jdneo
Copy link
Collaborator

jdneo commented Apr 8, 2021

Have no idea why CI fails. BTW, @LeonovecSergey do you know how we can verify this change manually(like the steps using the Bazel extension to play around)?

I use debug breakpoints to track java extension and build file patterns changes

  1. Open vscode-java fork in VSCode
  2. Launch as Extension
  3. In the opened window open some maven or gradle project (e.g. https://github.com/gradle/gradle-build-scan-quickstart)
  4. Update project (e.g shift+alt+U) for watching current state of java extensions and build file patterns
  5. Install new VSCode Extension that will add java extension or build file patterns from contributes in package.json (You can find vsix file in attached zip document)
  6. Click on the pop-up Reload button
  7. Update project (e.g shift+alt+U) for watching current state of java extensions and build file patterns
    bazel-ls-vscode-0.0.1.zip

Thanks for the detailed steps, will have a try!

@jdneo
Copy link
Collaborator

jdneo commented Apr 9, 2021

It works fine, when I installed the private build, the dialog pops up to ask for reloading.

Just one more question, I used this project for testing https://github.com/bmuschko/bazel-examples/tree/master/java/junit4-test, after reload, the classpath seems still not correct(red squiggles in the code), any steps I missed?

@LeonovecSergey
Copy link
Contributor Author

It works fine, when I installed the private build, the dialog pops up to ask for reloading.

Just one more question, I used this project for testing https://github.com/bmuschko/bazel-examples/tree/master/java/junit4-test, after reload, the classpath seems still not correct(red squiggles in the code), any steps I missed?

It is normal that the plugin did not resolve the classpath. There're two reasons for this:

  1. Our plugin supports a specific project structure only
    e.g. https://github.com/salesforce/bazel-ls-demo-project/tree/master/small
    And also you need to add some settings to the setting.json file:
    "java.import.bazel.enabled": true,
    "java.import.bazel.src.path": "/java/src",
    "java.import.bazel.test.path": "/java/test"
  2. The plugin used new features of eclipse.jdt.ls, which are not currently available in the release version.
    Enhanced IBuildSupport usage to support other build tools such as bazel eclipse-jdtls/eclipse.jdt.ls#1694

Refactoring

Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
src/plugin.ts Show resolved Hide resolved
Signed-off-by: Siarhei Leanavets <Siarhei_Leanavets1@epam.com>
@jdneo
Copy link
Collaborator

jdneo commented Apr 17, 2021

The problem fixed. It can pop-up the dialog for reloading when contribution changes.

Though I still cannot import the sample project, there is an error saying:

[Info  - 10:59:47 AM] Apr 17, 2021, 10:59:47 AM Tried to link a non-existant file [/Users/neo/Downloads/bazel-ls-demo-project-master/small/BUILD] for project [Bazel Workspace (small)]
[Error - 10:59:47 AM] Apr 17, 2021, 10:59:47 AM Initialization failed 
java.lang.NullPointerException
java.lang.RuntimeException: java.lang.NullPointerException
	at com.salesforce.b2eclipse.config.BazelEclipseProjectFactory.precomputeBazelAspectsForWorkspace(BazelEclipseProjectFactory.java:601)
	at com.salesforce.b2eclipse.config.BazelEclipseProjectFactory.importWorkspace(BazelEclipseProjectFactory.java:175)
	at com.salesforce.b2eclipse.managers.BazelProjectImporter.importToWorkspace(BazelProjectImporter.java:81)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.importProjects(ProjectsManager.java:113)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.initializeProjects(ProjectsManager.java:102)
	at org.eclipse.jdt.ls.core.internal.handlers.InitHandler$1.runInWorkspace(InitHandler.java:187)
	at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:42)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Caused by: java.lang.NullPointerException
	at com.salesforce.b2eclipse.config.BazelEclipseProjectFactory.precomputeBazelAspectsForWorkspace(BazelEclipseProjectFactory.java:597)
	... 7 more

Anyway, this is another problem beyond the scope of this PR. Overall the PR LGTM now.

Add @testforstephen @rgrunber @fbricon for awareness

@fbricon fbricon merged commit a8fef50 into redhat-developer:master Apr 20, 2021
@fbricon
Copy link
Collaborator

fbricon commented Apr 20, 2021

Thanks @LeonovecSergey!

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.

How can I add bazel support?
3 participants