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

8267418: IntelliJ build and test of JavaFX does not work #506

Closed
wants to merge 5 commits into from

Conversation

@Maran23
Copy link
Member

@Maran23 Maran23 commented May 19, 2021

Question: I was wondering, should I create a ticket for this as well? Given the fact that I don't have an https://bugs.openjdk.java.net account, I need to use the official bug reporting tool, which looked a bit overkill to me since someone needs to check my created ticket, while this PR is only affecting the IntelliJ IDE with OpenJFX and not the JavaFX platform directly.
EDIT: Thank you, Kevin. :)

This PR fixes the errors you get when cloning and working with OpenJFX in IntelliJ IDE:

  • The .idea/misc.xml is modified to use JDK_11 as language level instead of JDK_8.
    -> This is the language level shown inside the Project Structure. (File -> Project Structure...)
  • The .idea/base.iml, .idea/controls.iml, .idea/fxml.iml, .idea/web.iml, .idea/graphics.iml are modified to include/recognize the shims (as test resource, this is very similar to the configuration inside the .classpath file from Eclipse)
  • EDIT: The projects are now recognized by IntelliJ-gradle (.idea/gradle.xml, .idea/compiler.xml)

-> With this, I can run all normal tests with IntelliJ

What I couldn't fix yet (When I tried, it looked like IntelliJ is overriding the settings on next gradle reload):

  • IntelliJ is not detecting javafx.graphic inside the shims
  • All javafx.* dependencies are not found for the system tests

-> If someone has a solution, feel free to comment :)


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8267418: IntelliJ build and test of JavaFX does not work

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/506/head:pull/506
$ git checkout pull/506

Update a local copy of the PR:
$ git checkout pull/506
$ git pull https://git.openjdk.java.net/jfx pull/506/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 506

View PR using the GUI difftool:
$ git pr show -t 506

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/506.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 19, 2021

👋 Welcome back Maran23! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 19, 2021

Question: I was wondering, should I create a ticket for this as well?

All fixes need a bug ID in JBS. For this one, it seemed easier for me to just file it directly, so I did: JDK-8267418.

@kevinrushforth kevinrushforth requested a review from arapte May 19, 2021
@Maran23 Maran23 changed the title Make IntelliJ build/test working 8267418: IntelliJ build and test of JavaFX does not work May 19, 2021
@openjdk openjdk bot added the rfr label May 19, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 19, 2021

Copy link
Member

@arapte arapte left a comment

Is a specific version of IntelliJ required to test this fix, I use 2019.2 version. I always build using terminal, on my IntelliJ the build fails both before and after.

</GradleProjectSettings>
</option>
</component>
</project>
Copy link
Member

@arapte arapte Jun 1, 2021

Choose a reason for hiding this comment

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

I can see the same modules in project structure with or without this change. Is this required for build to work ?

Copy link
Member Author

@Maran23 Maran23 Jun 2, 2021

Choose a reason for hiding this comment

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

This enabled the IntelliJ gradle plugin for me, so e.g. the support for doing the tasks in the ide. And with it, I was able to run normal unit tests from the ide (see also screenshot(s) below)
image

.idea/modules.xml Outdated Show resolved Hide resolved
@Maran23
Copy link
Member Author

@Maran23 Maran23 commented Jun 2, 2021

Is a specific version of IntelliJ required to test this fix, I use 2019.2 version. I always build using terminal, on my IntelliJ the build fails both before and after.

I'm using the newest version (Community edition, 2021.1.2).

Executing gradle via the IntelliJ build-in terminal works for me:
image

Executing gradle sdk via 'IntelliJ Gradle' works for me:
image

And running normal unit tests works for me as well:
image

Any chance you can test it with the newest version?

@arapte
Copy link
Member

@arapte arapte commented Jun 7, 2021

I tested with IntelliJ IDEA 2021.1.2 (Community Edition) Build #IC-211.7442.40, built on June 1, 2021.

I see no side effects of this change. However, I also don't see what you see too.
I tried it with a fresh clone of repository.
The build from IntelliJ's inbuilt terminal worked without this change too, that terminal picks JDK/gradle/ant that was set in bashrc.

I also don't see the modules in gradle window. Here is the screenshot that I see when the project(with this PR change) is loaded for first time in the IntelliJ.
image

Is it possible that some local changes on your machine/repo result in what you see?
Some idea files are ignored using .gitignore file, so they get modified they won't be shown in 'git status'
from .gitignore file

# Ignore IntelliJ files
.idea/tasks.xml
.idea/workspace.xml
.idea/inspectionProfiles/**
.idea/copyright/profiles_settings.xml
.idea/codeStyles/**
.idea/checkstyle-idea.xml

It would be helpful to compare those files before and after loading the project in IntelliJ.
Also, can you test the PR once, by cloning a fresh copy of repo and then apply the PR.

@Maran23
Copy link
Member Author

@Maran23 Maran23 commented Jun 10, 2021

I tested with IntelliJ IDEA 2021.1.2 (Community Edition) Build #IC-211.7442.40, built on June 1, 2021.

I see no side effects of this change. However, I also don't see what you see too.
I tried it with a fresh clone of repository.
The build from IntelliJ's inbuilt terminal worked without this change too, that terminal picks JDK/gradle/ant that was set in bashrc.

I also don't see the modules in gradle window. Here is the screenshot that I see when the project(with this PR change) is loaded for first time in the IntelliJ.
image

This might not work because of the dependency verification error you have in your console. I had this as well and I temporary deleted the verification-metadata.xml inside the gradle folder. As stated in the mailing list you can also delete the .gradle cache and native folder, but that's the easier way to test it though. Then you can click on the refresh icon in the gradle window and everything should build and then the projects should be visible in the gradle window.

@arapte
Copy link
Member

@arapte arapte commented Jun 14, 2021

This might not work because of the dependency verification error you have in your console. I had this as well and I temporary deleted the verification-metadata.xml inside the gradle folder.

Deleting verification-metadata.xml temporarily solves the error on my system. The modules and tasks get listed in gradle window. Could execute graphics:test task from the gradle window.
The step to delete verification-metadata.xml seems acceptable. (May be we should include the whole IntelliJ setup steps with build documentation.)
It does not seem like a stopper to me, but would like to get @kevinrushforth 's opinion.

@arapte
Copy link
Member

@arapte arapte commented Jun 15, 2021

From the gradle documentation: Skipping Javadocs and sources
The verification of auto downloaded *-sources.jar files can be skipped using following change. This way we can avoid deleting the verification-metadata.xml file.

diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml
index abacd0b05b..0a3d33726d 100644
--- a/gradle/verification-metadata.xml
+++ b/gradle/verification-metadata.xml
@@ -3,6 +3,9 @@
    <configuration>
       <verify-metadata>true</verify-metadata>
       <verify-signatures>false</verify-signatures>
+      <trusted-artifacts>
+         <trust file=".*-sources[.]jar" regex="true"/>
+      </trusted-artifacts>
    </configuration>
    <components>
       <component group="" name="ffmpeg-3.3.3" version="">

I tested this change in addition to the PR changes, dependency verification does not fail with this change but fails without.
Can you please verify and test this change preferably on a clean repo by creating a new IntelliJ project.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jun 15, 2021

Good find. If this works, it seems like a good compromise to me. We don't download any *-sources or *-javadoc artifacts in our build, but it seems that IDEs do.

If we add this exclusion, I think we should do it using separate bug ID and a separate PR (requiring two reviewers, since this is an area we need to be careful about). As part of that, we would need to update the gradle/README.txt file to let people know why the *-sources / *-javadoc exclusion is there.

@Maran23
Copy link
Member Author

@Maran23 Maran23 commented Jun 22, 2021

diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml
index abacd0b05b..0a3d33726d 100644
--- a/gradle/verification-metadata.xml
+++ b/gradle/verification-metadata.xml
@@ -3,6 +3,9 @@
    <configuration>
       <verify-metadata>true</verify-metadata>
       <verify-signatures>false</verify-signatures>
+      <trusted-artifacts>
+         <trust file=".*-sources[.]jar" regex="true"/>
+      </trusted-artifacts>
    </configuration>
    <components>
       <component group="" name="ffmpeg-3.3.3" version="">

I tested this change in addition to the PR changes, dependency verification does not fail with this change but fails without.
Can you please verify and test this change preferably on a clean repo by creating a new IntelliJ project.

Good catch indeed! This works like a charm. I think this is a good follow-up.

@arapte
Copy link
Member

@arapte arapte commented Jun 23, 2021

The follow on is reported here: JDK-8269244

arapte
arapte approved these changes Jun 28, 2021
Copy link
Member

@arapte arapte left a comment

Approving, I re-verified the change: Loading project in IntelliJ does not cause any other changes in idea files.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8267418: IntelliJ build and test of JavaFX does not work

Reviewed-by: arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 19 new commits pushed to the master branch:

  • c4cc998: 8196065: ListChangeListener getRemoved() returns items that were not removed.
  • 78179be: 8267554: Support loading stylesheets from data-URIs
  • 3fd4c97: 8234920: Add SpotLight to the selection of 3D light types
  • 063bfe8: 8264137: Suppress deprecation and removal warnings of internal methods
  • 8e11b94: 8268915: WebKit build fails with Xcode 12.5
  • 45786ac: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
  • 04493e5: 8165214: ListView.EditEvent.getIndex() does not return the correct index
  • 13cffba: 8269026: PasswordField doesn't render bullet character on Android
  • 171e484: 8267551: Support loading images from inline data-URIs
  • 98138c8: 8268219: hlsprogressbuffer should provide PTS after GStreamer update
  • ... and 9 more: https://git.openjdk.java.net/jfx/compare/5e6d4429159e3fab9ec0aab9720393850d179710...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jun 28, 2021
@Maran23
Copy link
Member Author

@Maran23 Maran23 commented Jun 28, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jun 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

@Maran23
Your change (at version 591bdac) is now ready to be sponsored by a Committer.

@arapte
Copy link
Member

@arapte arapte commented Jun 30, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

Going to push as commit a1be1d5.
Since your change was applied there have been 23 commits pushed to the master branch:

  • b0d1586: 8266224: GitHub actions: use gcc 10.3 on Linux
  • 12fb4da: 8269259: Remove obsolete apps, tests, and scripts
  • 50ed890: 8269424: Some SuppressWarnings annotations can be more localized
  • 98e5166: 8269244: [IDE] Dependency verification of *-sources.jar fails when doing gradle sync
  • c4cc998: 8196065: ListChangeListener getRemoved() returns items that were not removed.
  • 78179be: 8267554: Support loading stylesheets from data-URIs
  • 3fd4c97: 8234920: Add SpotLight to the selection of 3D light types
  • 063bfe8: 8264137: Suppress deprecation and removal warnings of internal methods
  • 8e11b94: 8268915: WebKit build fails with Xcode 12.5
  • 45786ac: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
  • ... and 13 more: https://git.openjdk.java.net/jfx/compare/5e6d4429159e3fab9ec0aab9720393850d179710...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jun 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

@arapte @Maran23 Pushed as commit a1be1d5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@Maran23 Maran23 deleted the intellij-ide-fixes branch Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants