Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 10:31:40 GMT, Aleksey Shipilev wrote: >> No, change is specific to Linux. Please >> see(https://github.com/openjdk/jdk/commit/69b96f9a1b4a959c6b86f41c2259d9e4d47c8ede). > > OK, so what you are saying is that the path that references `S_ISBLK` is > protected by `BLKGETSIZE64` that is guaranteed to be undefined on OS X? If > so, this is (awkwardly) fine. I think Vymon took the code from FileDispatcherImpl_size0, which has been compiling on macOS without issues. I don't mind if we fix the issue with this PR or just back it out and fix it with another PR. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include src/java.base/unix/native/libjava/io_util_md.c line 256: > 254: return -1; > 255: } > 256: #if defined(__linux__) && defined(BLKGETSIZE64) Looks okay although would be good to fix the indentation at L256 before you integrate. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 09:50:44 GMT, Aleksey Shipilev wrote: >> this change will include the below headers files to Linux only. >> #include >> #include > > src/java.base/unix/native/libjava/io_util_md.c line 39: > >> 37: #if defined(__linux__) >> 38: #include >> 39: #include > > Does Mac OS need `sys/stat.h`, though? No, change is specific to Linux. Please see(https://github.com/openjdk/jdk/commit/69b96f9a1b4a959c6b86f41c2259d9e4d47c8ede). - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:26:30 GMT, Alan Bateman wrote: >> this change will include the below headers files to Linux only. >> #include >> #include > > src/java.base/unix/native/libjava/io_util_md.c line 256: > >> 254: return -1; >> 255: } >> 256: #if defined(__linux__) && defined(BLKGETSIZE64) > > Looks okay although would be good to fix the indentation at L256 before you > integrate. done fixed the indentation - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:09:07 GMT, Vyom Tewari wrote: >> I am not the Mac expert, but i checked on my MacOS Laptop and i did not >> found "BLKGETSIZE64" defined in sys/stat.h. >> As Alan already told that i took the code from FileDispatcherImpl.size0 and >> followed the same pattern. > > I will fix in this PR only. i will do the changes for only "io_util_md.c". updated the PR as suggested. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 10:59:37 GMT, Vyom Tewari wrote: >> I think Vymon took the code from FileDispatcherImpl_size0, which has been >> compiling on macOS without issues. I don't mind if we fix the issue with >> this PR or just back it out and fix it with another PR. > > I am not the Mac expert, but i checked on my MacOS Laptop and i did not found > "BLKGETSIZE64" defined in sys/stat.h. > As Alan already told that i took the code from FileDispatcherImpl.size0 and > followed the same pattern. I will fix in this PR only. i will do the changes for only "io_util_md.c". - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 10:27:34 GMT, Vyom Tewari wrote: >> src/java.base/unix/native/libjava/io_util_md.c line 39: >> >>> 37: #if defined(__linux__) >>> 38: #include >>> 39: #include >> >> Does Mac OS need `sys/stat.h`, though? > > No, change is specific to Linux. Please > see(https://github.com/openjdk/jdk/commit/69b96f9a1b4a959c6b86f41c2259d9e4d47c8ede). OK, so what you are saying is that the path that references `S_ISBLK` is protected by `BLKGETSIZE64` that is guaranteed to be undefined on OS X? If so, this is (awkwardly) fine. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 10:44:10 GMT, Alan Bateman wrote: >> OK, so what you are saying is that the path that references `S_ISBLK` is >> protected by `BLKGETSIZE64` that is guaranteed to be undefined on OS X? If >> so, this is (awkwardly) fine. > > I think Vymon took the code from FileDispatcherImpl_size0, which has been > compiling on macOS without issues. I don't mind if we fix the issue with this > PR or just back it out and fix it with another PR. I am not the Mac expert, but i checked on my MacOS Laptop and i did not found "BLKGETSIZE64" defined in sys/stat.h. As Alan already told that i took the code from FileDispatcherImpl.size0 and followed the same pattern. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 08:46:15 GMT, Vyom Tewari wrote: > Original fix was intended to Linux specific. Current Fix will not impact > other platforms. Your original fix failed to be Linux specific, so given you never actually tested it on other platforms you don't know whether another part of the fix is also not actually Linux-specific. At a minimum ensure you have GitHub actions enabled so that your fixes get some basic cross-platform testing before they are integrated. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:21:34 GMT, Vyom Tewari wrote: >>> Hi Vyom, >>> >>> That fixes the include file problem but as this was obviously not tested on >>> non-Linux have you checked that the rest of the fix for 8266610 works okay >>> on non-Linux? >>> >>> Thanks, >>> David >> >> Hi David, >> >> Original fix was intended to Linux specific. Current Fix will not impact >> other platforms. >> >> There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) >> which test the new code but currently it is silently passing without running >> the test. This test requires root access. >> >> Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) >> >> Thanks, >> Vyom > >> @vyommani how are you going to test this? > > I tested locally at my Linux environment. we have a test > "java/nio/channels/FileChannel/BlockDeviceSize.java" but it requires root > privileged , i ran this as well as a root. Please have a look into > this(https://bugs.openjdk.java.net/browse/JDK-8150539). > @vyommani You can start tier1 CI build to make sure build passes with this > fix. can you please do let me know how to start tier1 CI build ? - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include somehow tests not working for me. https://github.com/vyommani/jdk/actions/runs/827989185 - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 13:01:45 GMT, Jayathirth D V wrote: > In internal CI with fix tier 1 builds are running fine. Ok i will integrate my changes now. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 08:46:15 GMT, Vyom Tewari wrote: >> Hi Vyom, >> >> That fixes the include file problem but as this was obviously not tested on >> non-Linux have you checked that the rest of the fix for 8266610 works okay >> on non-Linux? >> >> Thanks, >> David > >> Hi Vyom, >> >> That fixes the include file problem but as this was obviously not tested on >> non-Linux have you checked that the rest of the fix for 8266610 works okay >> on non-Linux? >> >> Thanks, >> David > > Hi David, > > Original fix was intended to Linux specific. Current Fix will not impact > other platforms. > > There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) > which test the new code but currently it is silently passing without running > the test. This test requires root access. > > Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) > > Thanks, > Vyom > @vyommani how are you going to test this? I tested locally at my Linux environment. we have a test "java/nio/channels/FileChannel/BlockDeviceSize.java" but it requires root privileged , i ran this as well as a root. Please have a look into this(https://bugs.openjdk.java.net/browse/JDK-8150539). - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include The fix to 8266610 caused a build failure on macOS. What is your plan to make sure this won't happen with this change? I would recommend enabling [GitHub Actions](https://docs.github.com/en/github/administering-a-repository/disabling-or-limiting-github-actions-for-a-repository) for the "vyommani/jdk" repo. I have run this PR through our CI: the tier1 tests are fine on all supported platforms. Please integrate this PR as soon as possible. Separately, please sort out the way you test your PRs. Read the error message that the failed GitHub Actions gave you: > Prerequisites > Actions jobs for this repository have been disabled by GitHub staff. If you > are the repository owner, you can contact support via github.com/contact for > more information. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 08:46:15 GMT, Vyom Tewari wrote: >> Hi Vyom, >> >> That fixes the include file problem but as this was obviously not tested on >> non-Linux have you checked that the rest of the fix for 8266610 works okay >> on non-Linux? >> >> Thanks, >> David > >> Hi Vyom, >> >> That fixes the include file problem but as this was obviously not tested on >> non-Linux have you checked that the rest of the fix for 8266610 works okay >> on non-Linux? >> >> Thanks, >> David > > Hi David, > > Original fix was intended to Linux specific. Current Fix will not impact > other platforms. > > There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) > which test the new code but currently it is silently passing without running > the test. This test requires root access. > > Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) > > Thanks, > Vyom @vyommani how are you going to test this? - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:21:34 GMT, Vyom Tewari wrote: >>> Hi Vyom, >>> >>> That fixes the include file problem but as this was obviously not tested on >>> non-Linux have you checked that the rest of the fix for 8266610 works okay >>> on non-Linux? >>> >>> Thanks, >>> David >> >> Hi David, >> >> Original fix was intended to Linux specific. Current Fix will not impact >> other platforms. >> >> There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) >> which test the new code but currently it is silently passing without running >> the test. This test requires root access. >> >> Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) >> >> Thanks, >> Vyom > >> @vyommani how are you going to test this? > > I tested locally at my Linux environment. we have a test > "java/nio/channels/FileChannel/BlockDeviceSize.java" but it requires root > privileged , i ran this as well as a root. Please have a look into > this(https://bugs.openjdk.java.net/browse/JDK-8150539). @vyommani You can start tier1 CI build to make sure build passes with this fix. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 07:39:44 GMT, David Holmes wrote: > Hi Vyom, > > That fixes the include file problem but as this was obviously not tested on > non-Linux have you checked that the rest of the fix for 8266610 works okay on > non-Linux? > > Thanks, > David Hi David, Original fix was intended to Linux specific. Current Fix will not impact other platforms. There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) which test the new code but currently it is silently passing without running the test. This test requires root access. Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) Thanks, Vyom - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include MacOS builds fail with GHA since recently. So enabling GHA would serve a basic test for this to work. src/java.base/unix/native/libjava/io_util_md.c line 39: > 37: #if defined(__linux__) > 38: #include > 39: #include Does Mac OS need `sys/stat.h`, though? - PR: https://git.openjdk.java.net/jdk/pull/3943
RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
this change will include the below headers files to Linux only. #include #include - Commit messages: - fixed the indentation - added the additional check so that modified code get executed on linux only. - JDK-8266797: Fix for 8266610 breaks the build on macos Changes: https://git.openjdk.java.net/jdk/pull/3943/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3943=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266797 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3943.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3943/head:pull/3943 PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include Marked as reviewed by jdv (Reviewer). In internal CI with fix tier 1 builds are running fine. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include Hi Vyom, That fixes the include file problem but as this was obviously not tested on non-Linux have you checked that the rest of the fix for 8266610 works okay on non-Linux? Thanks, David The /test functionality doesn't work. You need to either test locally or using GitHub actions. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3943
