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

Remove IDE specific files from the source code repository (JDK-8198795) #4

Open
eugener opened this issue Feb 28, 2018 · 26 comments

Comments

@eugener
Copy link
Collaborator

commented Feb 28, 2018

Support for Gradle is pretty advanced in most modern IDEs, which means they can import the modules based just on Gradle project definition. So I propose to remove IDE specific files from the repo. This is the best practice anyway.

https://bugs.openjdk.java.net/browse/JDK-8198795

@eugener

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 28, 2018

This is relatively "low hanging fruit" so the process of reviewing and pushing fixes back to OpenJDK repo can be proved here.

@Ciruman

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

My college @streifi and I wanted to remove all IDE specific code and we found out that the IDEs(Netbeans, Eclipse and IntelliJ) couldn´t import the project from gradle. We had a look at the problem and we have found the problem in the gradle.build has a few errors(the dependencies are not correctly specified). We fixed it and now Netbeans and IntelliJ can import openjfx without a problem. Eclipse cannot import gradle/jigsaw projects yet.

We would like to push first this build fix and then remove the IDE files(and ignore them).

Pull request: #393
New one: #401

@DJViking

This comment has been minimized.

Copy link

commented Mar 1, 2019

We fixed it and now Netbeans and IntelliJ can import openjfx without a problem. Eclipse cannot import gradle/jigsaw projects yet.

Eclipse can import Gradle projects just fine with Eclipse Gradle Buildship. I have no problem with using Eclipse with Gradle on Eclipse 2018-12, Gradle 5.x and Buildship 3.0.
Though I have an eclipse.gradle with additional eclipse configuration to get it working with jigsaw.

eclipse {
    project {
        natures 'org.eclipse.buildship.core.gradleprojectnature'
    }

    classpath {
        file {
            whenMerged {
                entries.findAll { isModule(it) }.each { //(1)
                    it.entryAttributes['module'] = 'true'
                }

                entries.findAll { isSource(it) && isTestScope(it) }.each {
                    it.entryAttributes['test'] = 'true'
                }

                entries.findAll { isLibrary(it) && isTestScope(it) }.each {
                    it.entryAttributes['test'] = 'true'
                }
            }
        }

        downloadSources = true
        downloadJavadoc = true
    }
}

boolean isLibrary(entry) { return entry.properties.kind.equals('lib') }
boolean isTestScope(entry) { return entry.entryAttributes.get('gradle_used_by_scope').equals('test'); }
boolean isModule(entry) { isLibrary(entry) && !isTestScope(entry); }
boolean isSource(entry) { return entry.properties.kind.equals('src'); }

I just tried with openjdk-jfx, while I have no problem with my own projects, this one did not import well. There was a few problems. Let me just take one for example. base, module-info.java complained about "The package com.sun.javafx.runtime does not exist or is empty".

Have anyone talked to the Eclipse team on fixing the jigsaw support.
Seems there needs fix for both Eclipse Gradle Buildship and Gradle Eclipse plugin:
eclipse/buildship#658

For jigsaw I would anyway recommend a newer version of Gradle as it has better support for Java 11. Currently this project uses Gradle 4.8. However updating the gradle wrapper on this project was not hazzle free.

@Ciruman

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@DJViking Can you have a look to the pull request? I think if you want to try to import openjfx to eclipse, you need to use our pull request.

Cheers!

@DJViking

This comment has been minimized.

Copy link

commented Mar 4, 2019

I cloned the repository from the PR #393 and tried to import to Eclipse.
First I deleted all existing Eclipse IDE files (.project, .classpath), then imported by Gradle Buildship.

There is some shortcomings from the Eclipse Gradle support.
Gradle Buildship does not set test=true for the test sources, thus it complains about cannot resolve junit

<attribute name="test" value="true"/>

Gradle Buildship needs the "Project and External Dependencies" under Libraries on Classpath, and not Modulepath (not sure why).

I added the following to build.gradle to fix these issues:

apply plugin: "eclipse"
apply(from: rootProject.file('eclipse.gradle'))

There is only one showstopper left from getting javafx.base error free in eclipse:

exports com.sun.javafx.runtime to
    javafx.graphics;

Eclipse error: "The package com.sun.javafx.runtime does not exist or is empty"

Needed by test.com.sun.javafx.runtime.VersionInfoTest
import com.sun.javafx.runtime.VersionInfo;

Somehow this class is under src/main/version-info
https://github.com/javafxports/openjdk-jfx/blob/jfx-11/modules/javafx.base/src/main/version-info/VersionInfo.java

@Ciruman

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

If the pull request #401 is merged, IntelliJ and Netbeans will open openjfx without a problem. Eclipse seems to have a problem with Gradle 4.x and jigsaw.

If we create a pull request removing all IDE specific files, does it block eclipse developers? Do eclipse IDE files work(they did not work for us)?

I think this eclipse problematic should be handled in a different issue(@DJViking can create a new one with his findings).

@nlisker

This comment has been minimized.

Copy link

commented Mar 10, 2019

I have explained the Eclipse issue on the mailing list.

If we create a pull request removing all IDE specific files, does it block eclipse developers?

Yes. Eclipse files are not to be touched until Gradle can fully reproduce them. Have a look at the .classpath files for each project to see what Gradle needs to recreate.

Do eclipse IDE files work(they did not work for us)?

Yes. The .classpath files in the repo were specifically configured and are kept up to date. What does not work for you?

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Yes. Eclipse files are not to be touched until Gradle can fully reproduce them.

I completely agree with this.

I would go further and say that there needs to be general consensus among the users of NetBeans and IntelliJ that the generated-from-gradle project files can be used with no issues before any of those IDE files are removed.

@christianheilmann

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

The IntelliJ and Netbeans project files have not worked for me.

With the pullrequest JDK-8220222: specify clearly gradle project dependencies #401 we fixed the dependencies in the gradle file.
With this change, IntelliJ now can be used only with the gradle file. No IntelliJ project files are needed anymore.

Currently, Netbeans has an issue open with gradle and jigsaw (https://issues.apache.org/jira/browse/NETBEANS-2004 and kelemen/netbeans-gradle-project#417).

@Ciruman

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

With the pull request, NetBeans and IntelliJ do not have problems importing the project. Please, could anybody else test this? It would be great.

The problem in Netbeans is not that important, because the project is correctly imported but NetBeans, as @christianheilmann already mentioned, does not work 100% perfect with gradle and jigsaw. There is a false error shown but you can work without a problem.

We don't need to remove the eclipse files at this moment if this will block eclipse developers. But as soon as somebody check that the fix in the gradle.build is right, we could remove NetBeans files and IntelliJ files (and add them to the .gitignore). And as soon as eclipse works fine, we could consider removing them too.

@nlisker about Eclipse, I do not have a special interest in working with it but had a look at the options there were. Currently, with the change, we can use IntelliJ and NetBeans. Thank you for your interest. :-)

@Ciruman

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@kevinrushforth Does it make sense to ask about it in the mailing list?

@nlisker

This comment has been minimized.

Copy link

commented Mar 11, 2019

@Ciruman I was just explaining the situation because this was a discussion about removing all IDE files. :)
You also said that the files don't work for you, and they should, so I was wondering why.

@DJViking I think having Gradle recreate the Eclipse files is possible. It was done in this project and It works in terms of modulepath/classpath configurations. What needs to be addressed, though, is explicitly specifying reads/exports commands for each module that requires it.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Does it make sense to ask about it in the mailing list?

@Ciruman Yes, that's what I would recommend.

@DJViking

This comment has been minimized.

Copy link

commented Mar 11, 2019

@DJViking I think having Gradle recreate the Eclipse files is possible. It was done in this project and It works in terms of modulepath/classpath configurations.

Yes, I have done something very similar in my own projects as I posted in
#4 (comment)

Trying to import into Eclipse from #401
Import > Gradle > Existing Gradle Project
With Gradle Buildship I get these projects in Eclipse:

apps
base
controls
fxml
graphics
media
openjdk-jfx
swing
swt
systemTests
web

However importing through Gradle Buildship does not work completely, because it uses the Gradle Eclipse plugin and it recreates the .project and .classpath files and the are missing some important module and test attributes.
Unless you add in build.gradle my eclipse code snippet from #4 (comment) then it will work.


So if using the existing eclipse project files, then should use this import method instead:
Import > General > Existing Projects into Workspace

But need to import from modules directory, and not project root, or else you only get the root project openjdk-jfx.

Still there are problems with importing into Eclipse (Eclipse 2018-12).
This is for the javafx.base module:

  1. Problem with the existing eclipse project .classpath file.
    Project 'base' is missing required source folder: 'build/gensrc/java'Build Path Problem
    This source path does not exist
    Removed this from Java Build Path > Source

  2. Problem with module-info.java and a misplaced java class.
    One Error in src/main/java/module-info.java
    The package com.sun.javafx.runtime does not exist or is empty
    The test in src/test/java/test/com/sun/javafx/runtime/VersionInfoTest.java requires this also.

The class VersionInfo does exists, but lies under src/main/version-info/VersionInfo.java

@nlisker

This comment has been minimized.

Copy link

commented Mar 11, 2019

@DJViking But these are errors that arise from bad .classpath or configuration. After using your Gradle code, do you get the same .classpath files that currently exist?

@DJViking

This comment has been minimized.

Copy link

commented Mar 12, 2019

No, not quite the same .classpath files.
Some additional Gradle properties are added; gradle_scope, gradle_used_by_scope

This is the changes for base:

It removed source that does not exist:

- <classpathentry kind="src" path="build/gensrc/java"/>

Reading build.gradle I think I understand the purpose of version-info and build/gensrc/java
The VersionInfo.java is processed and placed in build/gensrc/java/com/sun/javafx/runtime
Gradle does not run processVersionInfo when importing the project.
Running ./gradlew processVersionInfo manually fixes that.
This can be fixed by adding to project(":base")

eclipseClasspath.dependsOn processVersionInfo

The main source gets additional output for shims:

- <classpathentry kind="src" path="src/main/java"/>
+ <classpathentry kind="src" output="bin/shims" path="src/main/java">
+     <attributes>
+         <attribute name="gradle_scope" value="shims"/>
+         <attribute name="gradle_used_by_scope" value="test,shims"/>
+     </attributes>
+ </classpathentry>

Not sure why, the src/main/java should actually be more like this:

<classpathentry kind="src" output="bin/main" path="src/main/java">
    <attributes>
        <attribute name="gradle_scope" value="main"/>
        <attribute name="gradle_used_by_scope" value="main,test"/>
    </attributes>
</classpathentry>

It is missing test attribute for source src/shims/java, and output is different:

- <classpathentry kind="src" output="testbin" path="src/shims/java">
-     <attributes>
-         <attribute name="test" value="true"/>
-     </attributes>
- </classpathentry>
+ <classpathentry kind="src" output="bin/shims" path="src/shims/java">
+     <attributes>
+         <attribute name="gradle_scope" value="shims"/>
+         <attribute name="gradle_used_by_scope" value="test,shims"/>
+     </attributes>
+ </classpathentry>

Missing optional attribute from src/test/java, and output is different:

- <classpathentry kind="src" output="testbin" path="src/test/java">
-     <attributes>
-         <attribute name="test" value="true"/>
-         <attribute name="optional" value="true"/>
-     </attributes>
- </classpathentry>
+ <classpathentry kind="src" output="bin/test" path="src/test/java">
+     <attributes>
+         <attribute name="gradle_scope" value="test"/>
+         <attribute name="gradle_used_by_scope" value="test"/>
+         <attribute name="test" value="true"/>
+     </attributes>
+ </classpathentry>

The output directory has changed:

- <classpathentry kind="output" path="bin"/>
+ <classpathentry kind="output" path="bin/default"/>

The JRE is missing the module attribute.

- <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER">
-     <attributes>
-         <attribute name="module" value="true"/>
-     </attributes>
- </classpathentry>
+ <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-11/"/>

The JUNIT is missing:

- <classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/4">
-     <attributes>
-         <attribute name="test" value="true"/>
-     </attributes>
- </classpathentry>

This can be added by setting extra container in gradle eclipse configuration, but it is actually not needed since Gradle Buildship adds the junit4 dependency.

@DJViking

This comment has been minimized.

Copy link

commented Mar 12, 2019

With the following additions to build.gradle I have been getting javafx.base module error free when imported with Gradle Buildship.

apply plugin: "eclipse"
apply(from: rootProject.file('eclipse.gradle'))
eclipseClasspath.dependsOn processVersionInfo

Only modules graphics and web that have remaining errors in eclipse.

@nlisker

This comment has been minimized.

Copy link

commented Mar 13, 2019

What are the Gradle attributes "gradle_scope" and "gradle_used_by_scope"?

I don't think output="bin/shims" needs to be specified for path="src/main/java".

Only modules graphics and web that have remaining errors in eclipse.

Graphics will require an add-exports:

<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER">
	<attributes>
		<attribute name="module" value="true"/>
		<attribute name="add-exports" value="java.base/sun.security.util=javafx.graphics"/>
	</attributes>
</classpathentry>

But controls also requires add-exports, so I'm not sure how it is working for you.

@DJViking

This comment has been minimized.

Copy link

commented Mar 13, 2019

What are the Gradle attributes "gradle_scope" and "gradle_used_by_scope"?

I'm not sure what these are for. They are added by Gradle Eclipse plugin.

Graphics will require an add-exports:

<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER">
	<attributes>
		<attribute name="module" value="true"/>
		<attribute name="add-exports" value="java.base/sun.security.util=javafx.graphics"/>
	</attributes>
</classpathentry>

But controls also requires add-exports, so I'm not sure how it is working for you.

Will check how to get this add-exports into the JRE container automatically from the Gradle Eclipse Plugin. Perhaps some missing configuration in build.gradle. Wiil look into this further.

Got the following add-exports in graphics .classpath

<classpathentry combineaccessrules="false" kind="src" path="/base">
	<attributes>
		<attribute name="module" value="true"/>
		<attribute name="add-exports" value="javafx.base/com.sun.javafx.property=javafx.graphics"/>
	</attributes>
</classpathentry>

Got the following add-exports in controls .classpath

<classpathentry kind="src" path="/graphics">
	<attributes>
		<attribute name="module" value="true"/>
		<attribute name="add-exports" value="javafx.graphics/test.com.sun.javafx.pgstub=javafx.controls"/>
	</attributes>
</classpathentry>
<classpathentry combineaccessrules="false" kind="src" path="/base">
	<attributes>
		<attribute name="module" value="true"/>
		<attribute name="add-exports" value="javafx.base/test.com.sun.javafx.binding=javafx.controls"/>
	</attributes>
</classpathentry>
@nlisker

This comment has been minimized.

Copy link

commented Mar 13, 2019

The add-exports you got for graphics and controls are correct. They apply to the other modules that this module depends on. Graphics indeed relies on javafx.base/com.sun.javafx.property. So, somehow the correct add-exports are applied to the dependent projects, but not the JRE.

@christianheilmann

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Our pull-request #401 checkin enables full working OpenJFX project in IntelliJ IDEA Version 20.18.3 or higher with the checked in build.gradle.

Simply open the build.gradle in IntelliJ and you are ready to code.

The checked in IntelliJ project files should be removed, because they do not work and are misleading!

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

I just approved PR #401 which can unblock this. As a next step, JDK-8198795 should be split into 3 separate issues, one for each of IntelliJ, Eclipse, and Netbeans. You will need general consensus from the users of an IDE before removing the checked in files for that IDE. By splitting it into three separate issues, you can take care of each IDE separately when the users of that IDE are happy with it.

@Ciruman

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I created the following tickets for Eclipse, Netbeans, and IntelliJ. I will create a PR to remove IntelliJ files.
@nlisker, if Eclipse is able to load the projects from the gradle files and therefore, Eclipse files are not needed anymore, I could also remove them in another PR.
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8223374
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8223375
https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8223373

@nlisker

This comment has been minimized.

Copy link

commented May 6, 2019

@Ciruman Eclipse is not able to create its files from Gradle yet. @DJViking worked on an attempt, but it's incomplete. Once we have a working solution I will remove the files.

@nlisker

This comment has been minimized.

Copy link

commented Jun 30, 2019

@DJViking Have you had a chance to figure out the JRE container add-exports issue?

@DJViking

This comment has been minimized.

Copy link

commented Sep 16, 2019

@nlisker Sorry I didn't reply. I was away for summer vacation and I simply forgot looking into this issue further.
I haven't had much the time to look more into this. I think I will take a look again soon, maybe this weekend and see what the current status is with openjfx and eclipse on Gradle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.