On Sat, 22 Oct 2022 07:49:12 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
> See https://bugs.openjdk.org/browse/JDK-8221708 This is the review for /apps. apps itself is a gradle project as specific in the root project build.gradle file. I think that a gradle file should be generated there and the gradle nature added to it (the removal of the java nature is correct), but as I'm having build errors with gradle, I don't see any. Review for /tests: * /tests itself, despite being a top level folder, is neither a gradle nor a java project, so its peoject and classpath files need to be removed. /tests/system is the project that is configured by the root project. * Will need to think how to treat /manual. There doesn't seem to be normal structure with source folders. The changes to the graphics module's classpath file should be removed. It was updated in a more recent PR. The changes here should only apply to apps and tests (unless I find something else to fix while on it). I'll go over each folder to see what's misconfigured after all this time. It looks like a lot of projects under tests weren't configured. You can also remove the classpath of the root project as it's not a java project, just a gradle one. In the project file, maybe its name can be changed to jfx. I'm not sure if the name is auto-generated by gradle as specified in the comment. This is lumping in https://bugs.openjdk.org/browse/JDK-8293185. Yes, looking at, e.g., `src/testapp7/java`, the `mymod` folder needs to be a source folder for the module-info to be placed correctly, but then `src/testapp7/resources` has to be placed in a different project. I don't see an Eclipse-level solution here. I think that the contents of the folder need to be rearranged. I'm using the wrapper, which is 7.3. I managed to solve is using `gradlew --write-verification-metadata sha256 help`. The manual says it should be run after updating a dependency, so I wonder why I'm the only one with this problem. Looks good. Waiting on tests/manual. We also need to check the viability of running the gradle projects with gradle. I will write to the mailing list later. In your screenshot I see that apps is a Java project. That is wrong already. Also, none of the modules were touched, so the errors are from somewhere else. apps is a project, but not a java project. The new screenshot looks more correct, but now `controls` is a gradle project for some reason even though the project fil doesn't define it as such. I can't tell what errors you get on the modules projects, but these should not be affected at all since they weren't touched. I imported all projects as Java projects (not through the Gradle import) and got no errors except for the missing SWT dependency in the manual test. In addition, I have tested removing the project files on apps and tests/system (the Gradle 2 projects defined in the root project). I saw no problem. Buildship knows how to read them from their parents, so they don't need to be Gradle projects themselves. The only difference is the icon displayed in the Gradle Tasks view. I suggest removing these 2 files. Another test I made is with regards to the root project name (part of https://bugs.openjdk.org/browse/JDK-8293185 that we can probably close as a duplicate of this). Changing it didn't seems to have an effect on functionality. However, it's confusing both for the user and for Buildship if the project name and the folder name are different. Buildship shows both of them, but one has an error marker on it, while the other is functioning normally. Since the root folder might be named differently for different users as it's a repository base, I don't know if there's a solution that works for everyone. I can only suggest to rename it to `jfx` because that's the default folder name (and remove the comment in the project file). Previously (on mercurial) it was `rt`, that's where the current name comes from. I'll give the new classpath files under /tests one more look, but aside from that the PR will be ready as far as I'm concerned. * > Missing Gradle project configuration file: .settings/org.eclipse.buildship.core.prefs These files are in this PR, for example, https://github.com/openjdk/jfx/pull/930/files#diff-4b7911fda8890aa89f0b972959f282b0fa10750a81f2f20a8ab253b492ac05d9. * > import as gradle project, following the standard recipe from https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX produces 45K errors. A modified procedure described below produces 20K errors. What errors are these? Where do they come from? If you are getting "___ cannot be resolved" errors as you have shown above, then check if the resource that has them is defined on that project. If it is, then I don't understand why you get them. Even more so, it looks like you are getting them on module projects, while module projects weren't touched in this PR. I have deleted all the project and imported them in various ways at least 10 times while reviewing and never got those errors. I suspect it's local. The question is if other new users will have these issues too. I tested both importing as Gradle and as Existing projects, even one directly after the other to see what happens. Once the Gradle-generated files are reverted to the ones in this PR, most errors are gone (there is still the SWT error, but we know why). I'm looking at the `javax.script.AbstractScriptEngine` error in your screenshot. It is part of the `java.scripting` module in the JDK. Go to the project build path and under the Module Dependencies tab check that you have that module listed:  1. I'm not aware of a way to that stops Buildship from creating Eclipse files. If we had, it would have saved some trouble long ago. I'll try to look into it. 2. I don't know what causes that. I did notice that in all my importing and reimporting new files were not created in the bin after I deleted them. This will also require looking into. 3. The wizard uses the same process as in point 1. I don't think the wizard is the problem. I think that the end goal is to allow users to choose what tools they want to work with. If someone doesn't want to use Gradle from within Eclipse (and only from the command line) that's fine. Same for someone who does want that. I think that I should clarify a few things, just to be on the safe side. Using Gradle is essential to the development process. Developers in Eclipse can choose to either use Gradle externally (from the command line), or use built-in Gradle integrations in Eclipse. Those who choose to use Gradle externally don't need Buildship (or whatever other Gradle plugin there is) at all and no config files like those in `.settings`. This should always be a viable options and I know developers of JavaFX that use this option. Those who choose to use Gradle from within Eclipse (Buildship allows the Task View, custom Gradle run configurations, multiple consoles...) either need the config files given to them, or have Buildship generate it for them. When Buildship generates these, it also generates the project/classpath files, which need to be reverted. I don't think it's possible to separate the processes. There were also sentiments against checking in Bulldship files in the past. As I see it, we have 3 paths: 1. Don't provide any Gradle files and let those who want to use Gradle in Eclipse generate files locally (either via a Gradle import or a Refresh Gradle Project), reverting the wrong ones. 2. Provide all the Gradle files and explicitly say not to generate your own files or you will mess up your workspace. If you don't use Gradle from Eclipse then you don't care about these files. 3. Modify the Gradle build file to generate the correct Eclipse files and remove all Eclipse files from the repo. This was discussed a few years ago in the previous jfx repo at https://github.com/javafxports/openjdk-jfx/issues/4. Perhaps it's worth taking a look at again. > I haven't been able to reproduce the bin problem in any way after I manually > deleted all of them. Same here. They might appear at some point later, though. > A few projects don't work correctly with Gradle (Ensemble8, 3DViewer and > Modena), need to decide how to handle this, either: a) Fix their > configuration in Gradle, or b) Remove the Gradle nature I don't use them myself. [Kevin commented](https://github.com/openjdk/jfx/pull/930#issuecomment-1293399112) about them. One cleanup pass has been made in [DK-8269259](https://bugs.openjdk.org/browse/JDK-8269259) and there should be another one, perhaps touching on these. If they stay, I think that Gradle tasks should be added to run these, including the 3DLighting project and maybe others. System tests could also use ready-made commands to run as specified [here](https://wiki.openjdk.org/display/OpenJFX/Building+OpenJFX#BuildingOpenJFX-RunningsystemtestswithRobot). This is probably for a different PR. > After everything works, we'll need to update the wiki I update it from time to time and I already have a draft writeup for its next update after we decide what we want to do about Gradle. apps/samples/3DViewer/.classpath line 14: > 12: <attribute name="test" value="true"/> > 13: </attributes> > 14: </classpathentry> These 2 look empty. If you don't see that these exist then they should be removed. apps/toys/ColorCube/.project line 4: > 2: <projectDescription> > 3: <name>ColorCube</name> > 4: <comment></comment> This change doesn't do anything as far as I know. It can be reverted. It can also stay. tests/system/.classpath line 153: > 151: </attributes> > 152: </classpathentry> > 153: <classpathentry kind="con" > path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-11/"/> I don't think this is right. No need to specify Java 11, just use the configured jdk as in all other projects. ------------- Changes requested by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/930
