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 eclipse support #590

Merged
merged 9 commits into from
Sep 26, 2022
Merged

Add eclipse support #590

merged 9 commits into from
Sep 26, 2022

Conversation

rougsig
Copy link
Collaborator

@rougsig rougsig commented Jul 26, 2022

More details here #596

@rougsig
Copy link
Collaborator Author

rougsig commented Aug 21, 2022

I leave IDESupportTest with fail. It's expected behaviour for now.

Open questions:

  1. When we set up eclipse with the eclipse plugin, we can add any attributes. With the Gradle vanilla API, we can't do that.
    Is it necessary to pass all the attributes that were in the test?

  2. The :eclipse task only includes existing folders in the .classpath. That is, this folder is registered as a sourceSet, but if the folder does not exist on disk, it will not be included in the .classpath file.
    Is it okay?

cc @jdneo @ejona86

@jdneo
Copy link

jdneo commented Aug 22, 2022

When we set up eclipse with the eclipse plugin, we can add any attributes. With the Gradle vanilla API, we can't do that.

Does Gradle vanilla API support a limit set of the attributes? Or no attributes are supported at all?

if the folder does not exist on disk, it will not be included in the .classpath file.

This is the purpose of the attributes optional & ignore_optional_problems. An optional classpath entry is allowed to be added into .classpath even it does not exist on the disk in Eclipse.

If the folder does not included in the .classpath file, after user compiles the proto files to Java sources, user needs to reload the project to update the .classpath file, otherwise, there will still be a lot of compilation errors displayed. That might not be ideal as it's not out-of-box experience - user might not be aware that such action is needed to make it work.

@rougsig
Copy link
Collaborator Author

rougsig commented Aug 22, 2022

Does Gradle vanilla API support a limit set of the attributes? Or no attributes are supported at all?

As i know - not supported to add any attributes. All required attributes will be set automatically.

That might not be ideal as it's not out-of-box experience - user might not be aware that such action is needed to make it work.

You are right.

Look like we need to keep custom logic for eclipse and idea.


@jdneo
I'm not familiar with eclipse. I used it a long time ago when the idea didn't exist yet. When does eclipse generate a .classpath file? Is it generated automatically or does the user need to execute a specific command?

@jdneo
Copy link

jdneo commented Aug 23, 2022

When does eclipse generate a .classpath file? Is it generated automatically or does the user need to execute a specific command?

It's generated automatically when user is importing a Gradle project in Eclipse. There is a plug-in in Eclipse called Buildship, this plugin is used to parse the Gradle project to an Eclipse project (just like when we import a Maven or Gradle project into IDEA, IDEA will try to understand the project and parse it into IDEA's own model).

The .classpath file is generated when Buildship is doing the import stuff.

@rougsig
Copy link
Collaborator Author

rougsig commented Aug 24, 2022

I moved the eclipse fix to this PR for the reason that registering the sourceset should fix the issue. Sourceset registration actually solves the problem, but not in the best way for the user experience. IDEA works well without custom code from the gradle idea plugin. Eclipse doesn't work perfectly without custom code from eclipse gradle plugin. I think the best solution will be keep custom code for eclipse.

@jdneo How do you feel better, will you merge #596 or will we leave these changes in this PR?

@jdneo
Copy link

jdneo commented Aug 25, 2022

I think the best solution will be keep custom code for eclipse.

Agree.

will you merge #596 or will we leave these changes in this PR?

I can append a commit to this PR (Not sure if I have the push permission to your fork). Or you can merge this one and I can update #596 based on the latest change.

@jdneo
Copy link

jdneo commented Aug 29, 2022

Hi @rougsig 👋,

I found I have no permission to append commit to your fork. So, I updated #596. (Two deprecated attributes are removed)

@rougsig rougsig changed the title Add source sets Add eclipse support Aug 29, 2022
- add the generated source folders to Eclipse classpath entries. Due to
  a limition of Buildship, we have to create those folders beforehand,
  see: eclipse/buildship#1196

Signed-off-by: sheche <[email protected]>
@rougsig
Copy link
Collaborator Author

rougsig commented Aug 29, 2022

I keep for now idea custom code.

@jdneo I fix lint problems, and move IDE support tests to separate file. All other changes is yours. Remove draft status.

@rougsig rougsig marked this pull request as ready for review August 29, 2022 10:38
@rougsig
Copy link
Collaborator Author

rougsig commented Aug 29, 2022

@jdneo Could you review this PR?

@jdneo
Copy link

jdneo commented Aug 30, 2022

Thank you @rougsig, I'll review it today.

Copy link

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

LGTM!

Now Eclipse and VS Code can automatically add the protobuf output source directories into the source paths, without manually & explicitly set the source set in build.gradle.

Eclipse

protobuf-eclipse

VS Code

protobuf-vscode

Copy link
Collaborator

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Sourceset registration actually solves the problem, but not in the best way for the user experience. ... Eclipse doesn't work perfectly without custom code from eclipse gradle plugin.

What is wrong with Eclipse support when we don't have the custom code? What is wrong with having code that just creates the directories, and nothing else?

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