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

8241476: Linux build warnings issued on gcc 9 #150

Closed
wants to merge 10 commits into from

Conversation

@tsayao
Copy link
Collaborator

@tsayao tsayao commented Mar 24, 2020

Simple fix to remove annoying warnings.


Progress

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

Issue

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Ambarish Rapte (arapte - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/150/head:pull/150
$ git checkout pull/150

tsayao added 5 commits Jan 23, 2020
Merge master
Merge from jfx
Merge
merge from jfx
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 24, 2020

👋 Welcome back tsayao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Mar 24, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2020

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The problem is that gcc, for whatever reason, started issuing a (useless) warning if you pass the -Werror=implicit-function-declaration option to gcc for C++ files. I don't like the solution of removing that flag for C files. I think the better solution will be to have a set of options that are common to both C and C++ (all but this one currently), and then add -Werror=implicit-function-declaration only to the options for C. Until then, I think we live with this warning. It's better than losing the checking for C files.

tsayao added 3 commits Mar 25, 2020
Merge upstream
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Mar 25, 2020

Like this?

I think all C builds are covered. Gstreamer seems to have a Makefile with the flags. Not sure about libxml and libxlst inside javafx.web.

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Mar 26, 2020

I think it's better to split the flags between CFLAGS and CPPFLAGS (as is done in OpenJDK). The -Werror=implicit-function-declaration is extremely useful for C files, and we don't want to risk that this somehow got lost.
If we use CFLAGS and CPPFLAGS, we can pass the latter as CXXFLAGS to cmake, so that should be fairly easy.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 26, 2020

I took a look yesterday and came to the same conclusion that what we really want are separate C and C++ flags. For now, the only difference would be the presence or absence of -Werror=implicit-function-declaration, but it would allow for other differences in the future if needed.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 26, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Mar 26, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Mar 31, 2020

Please, let me know if this is the desired way to do it. If not, I will rework it. Thanks.

@tsayao tsayao requested a review from kevinrushforth Apr 2, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 7, 2020

This is roughly what I had in mind. I want to test it, and also take a look at the WebKit and media builds as well. WebKit uses the flags from linux.gradle, and is mostly C++, so I think it needs no changes. Media does not use any flags from linux.gradle -- only the jfxmedia project has C++ files on Linux. I guess it should be fine, too, but I'll see.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The build changes in linux.gradle look good. I tested it, and all warnings are gone (including those in WebKit) except for the ones in the jfxmedia sub-project of javafx.media.

Two comments:

  1. I changed the bug title in JBS to refer to gcc 9 rather than Ubuntu to reflect the actual cause of the additional warnings. Can you update the title of this PR to match?
8241476: Linux build warnings issued on gcc 9
  1. Can you add the following change to your PR as well? This will fix the remaining warnings in jfxmedia:
--- a/modules/javafx.media/src/main/native/jfxmedia/projects/linux/Makefile
+++ b/modules/javafx.media/src/main/native/jfxmedia/projects/linux/Makefile
@@ -41,7 +41,6 @@ ifdef HOST_COMPILE
                   -Wextra \
                   -Wformat-security \
                   -fstack-protector \
-                  -Werror=implicit-function-declaration \
                   -Werror=trampolines \
                  -msse2 \
                  -DGSTREAMER_LITE
@tsayao tsayao changed the title 8241476: Linux build warning issued on updated compilers (Ubuntu 18.04.4 / 20.04) 8241476: Linux build warnings issued on gcc 9 Apr 10, 2020
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Apr 10, 2020

Done.

@tsayao tsayao closed this Apr 10, 2020
@tsayao tsayao reopened this Apr 10, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good.

@kevinrushforth kevinrushforth requested a review from johanvos Apr 10, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 13, 2020

@johanvos or @arapte Can one of you be the second reviewer on this?

@kevinrushforth kevinrushforth mentioned this pull request Apr 13, 2020
3 of 3 tasks complete
@arapte
Copy link
Member

@arapte arapte commented Apr 14, 2020

After this change below c files would be compiled without -Werror=implicit-function-declaration option.
modules/javafx.media/src/main/native/jfxmedia/Utils/ColorConverter.c

modules/javafx.graphics/src/main/native-glass/gtk/wrapped.c
modules/javafx.graphics/src/main/native-glass/gtk/launcher.c

This can be addressed in follow on issue. Recording the files here for track.
Looks good to me.

@arapte
arapte approved these changes Apr 14, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2020

@tsayao This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8241476: Linux build warnings issued on gcc 9

Reviewed-by: kcr, arapte
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 32 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 9d50c4cc01856c0c6a07fa27e4258216c2f2d0e8.

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 (@kevinrushforth, @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 Apr 14, 2020
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Apr 14, 2020

/integrate

@openjdk openjdk bot added the sponsor label Apr 14, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 14, 2020

@tsayao
Your change (at version 92f1b9a) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 15, 2020

/sponsor

@openjdk openjdk bot closed this Apr 15, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Apr 15, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2020

@kevinrushforth @tsayao The following commits have been pushed to master since your change was applied:

  • 9d50c4c: Merge
  • 4d69a0d: 8241629: [macos10.15] Long startup delay playing media over https on Catalina
  • b1fdc45: 8242209: Increase web native thread stack size for x86 mode
  • e1cb191: 8240694: [macos 10.15] JavaFX Media hangs on some video files on Catalina
  • c154538: 8242106: [macos] Remove obsolete GlassView2D.m class
  • 3f663e3: 8240262: iOS refresh rate is capped to 30 Hz
  • 231879a: 8241710: NullPointerException while entering empty submenu with "arrow right"
  • 470c7d0: 8230809: HTMLEditor formatting lost when selecting all (CTRL-A)
  • fda015c: 8242167: ios keyboard handling
  • 844460b: 8242163: Android keyboard integration fails
  • 364c64a: 8241249: NPE in TabPaneSkin.perfromDrag
  • 418675a: 8236840: Memory leak when switching ButtonSkin
  • 247a65d: 8236971: [macos] Gestures handled incorrectly due to missing events
  • 560ef17: 8241455: Memory leak on replacing selection/focusModel
  • ef37669: Merge
  • 5906521: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034
  • 159f651: 8240542: Switch FX build to use JDK 14 as boot JDK
  • 6d098fe: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
  • d7f13f4: 8089828: RTL Orientation, the flag of a mnemonic is not placed under the mnemonic letter.
  • 9ecc107: 8240539: Upgrade gradle to version 6.3
  • f3a3ea0: 8234471: Canvas in webview displayed with wrong scale on Windows
  • f2bca9f: Merge
  • 6900d29: Merge
  • e91bec4: Merge
  • 66a8f49: Merge
  • fde42da: Merge
  • e21fd1f: Merge
  • 443c845: Merge
  • 31e63de: Merge
  • 14c6938: 8236798: Enhance FX scripting support
  • bfb2d0e: Merge
  • 39f6127: Merge

Your commit was automatically rebased without conflicts.

Pushed as commit 7044cef.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants