Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Thu, 26 Nov 2020 16:25:35 GMT, Magnus Ihse Bursie wrote: >>> That sounds very good indeed! I did not think it was possible for Skara to >>> access the Checks area, but if it does, it's really where this belong. >> >> I've tried this out a bit in the playground now - perhaps something like >> this: https://github.com/openjdk/playground/pull/77 > > @rwestberg Looks sooo much better! What happens if a build fails? I think there were a few failing test jobs in the current state of the playground, so should be visible in a little while! - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Thu, 26 Nov 2020 16:07:54 GMT, Robin Westberg wrote: >> @rwestberg That sounds very good indeed! I did not think it was possible for >> Skara to access the Checks area, but if it does, it's really where this >> belong. > >> That sounds very good indeed! I did not think it was possible for Skara to >> access the Checks area, but if it does, it's really where this belong. > > I've tried this out a bit in the playground now - perhaps something like > this: https://github.com/openjdk/playground/pull/77 @rwestberg Looks sooo much better! What happens if a build fails? - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Mon, 23 Nov 2020 11:54:48 GMT, Magnus Ihse Bursie wrote: > That sounds very good indeed! I did not think it was possible for Skara to > access the Checks area, but if it does, it's really where this belong. I've tried this out a bit in the playground now - perhaps something like this: https://github.com/openjdk/playground/pull/77 - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Mon, 23 Nov 2020 10:09:08 GMT, Robin Westberg wrote: >> Yeah, bike-shedding aside, "Linux additional" is probably fine. So if we >> accept #1379, then it would also add "Windows additional"? > > Ah yeah, just saw that one. I guess just one additional column will still > make the table too wide.. Perhaps all the cross builds should just share a > single column. :) > > Going forward, I do think that the table should perhaps be removed though, > and the information should instead be moved down to the "checks" box (just as > GitHub actions show them natively - but perhaps condense them a bit just as > the current table). The constant updating of the table is probably the major > source of the "edit war" you have noticed can occur if you want to edit the > PR body yourself while any GitHub actions are still in progress. @rwestberg That sounds very good indeed! I did not think it was possible for Skara to access the Checks area, but if it does, it's really where this belong. - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Mon, 23 Nov 2020 09:59:39 GMT, Aleksey Shipilev wrote: >> I personally think I prefer to just use a single column for the additional >> cross compile builds, because the table becomes very big otherwise (and the >> rows get split up). You still see the full name of the build in case >> anything fails, so not sure it adds that much value to get an additional box >> for each cross-compiled architecture, I guess in 99% of cases they will just >> pass and then you don't really care exactly which platforms you didn't >> break.. But I'm no UX expert. :) > > Yeah, bike-shedding aside, "Linux additional" is probably fine. So if we > accept #1379, then it would also add "Windows additional"? Ah yeah, just saw that one. I guess just one additional column will still make the table too wide.. Perhaps all the cross builds should just share a single column. :) Going forward, I do think that the table should perhaps be removed though, and the information should instead be moved down to the "checks" box (just as GitHub actions show them natively - but perhaps condense them a bit just as the current table). The constant updating of the table is probably the major source of the "edit war" you have noticed can occur if you want to edit the PR body yourself while any GitHub actions are still in progress. - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Mon, 23 Nov 2020 09:55:34 GMT, Robin Westberg wrote:
>> .github/workflows/submit.yml line 526:
>>
>>> 524: echo "CC=${{ matrix.gnu-arch }}-linux-gnu${{
>>> matrix.gnu-flavor}}-gcc-10" >> $GITHUB_ENV
>>> 525: echo "CXX=${{ matrix.gnu-arch }}-linux-gnu${{
>>> matrix.gnu-flavor}}-g++-10" >> $GITHUB_ENV
>>> 526: echo "cross_flags=--openjdk-target=${{ matrix.gnu-arch
>>> }}-linux-gnu${{ matrix.gnu-flavor}} --with-sysroot=${HOME}/sysroot-${{
>>> matrix.debian-arch }}/ --with-toolchain-path=${HOME}/sysroot-${{
>>> matrix.debian-arch }}/ --with-freetype-lib=${HOME}/sysroot-${{
>>> matrix.debian-arch }}/usr/lib/${{ matrix.gnu-arch }}-linux-gnu${{
>>> matrix.gnu-flavor}}/ --with-freetype-include=${HOME}/sysroot-${{
>>> matrix.debian-arch }}/usr/include/freetype2/
>>> --x-libraries=${HOME}/sysroot-${{ matrix.debian-arch }}/usr/lib/${{
>>> matrix.gnu-arch }}-linux-gnu${{ matrix.gnu-flavor}}/" >> $GITHUB_ENV
>>
>> No chance to break this line by option? It is kinda hard to read and thus
>> verify. It is still okay if we cannot.
>
> Sure, I agree, perhaps just putting that as a separate step would increase
> readability.
The update looks fine.
-
PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Mon, 23 Nov 2020 09:54:52 GMT, Robin Westberg wrote: >> Ah wait, I now see "Linux additional" is the column name in testing table, >> because it is the name of the job! Eh... It was nicer to have columns per >> arch. Does it make sense to split the "Linux x64 (other)" and "Linux >> Foreign" then? It might also simplify the coding as we would not have to >> check for `matrix.debian_arch`. >> >> EDIT: Or maybe it is fine to have "Linux additional", as it stand now. >> Really undecided here :) > > I personally think I prefer to just use a single column for the additional > cross compile builds, because the table becomes very big otherwise (and the > rows get split up). You still see the full name of the build in case anything > fails, so not sure it adds that much value to get an additional box for each > cross-compiled architecture, I guess in 99% of cases they will just pass and > then you don't really care exactly which platforms you didn't break.. But I'm > no UX expert. :) Yeah, bike-shedding aside, "Linux additional" is probably fine. So if we accept #1379, then it would also add "Windows additional"? - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Fri, 20 Nov 2020 17:31:08 GMT, Aleksey Shipilev wrote:
>> Robin Westberg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve naming, fix style issues
>
> .github/workflows/submit.yml line 526:
>
>> 524: echo "CC=${{ matrix.gnu-arch }}-linux-gnu${{
>> matrix.gnu-flavor}}-gcc-10" >> $GITHUB_ENV
>> 525: echo "CXX=${{ matrix.gnu-arch }}-linux-gnu${{
>> matrix.gnu-flavor}}-g++-10" >> $GITHUB_ENV
>> 526: echo "cross_flags=--openjdk-target=${{ matrix.gnu-arch
>> }}-linux-gnu${{ matrix.gnu-flavor}} --with-sysroot=${HOME}/sysroot-${{
>> matrix.debian-arch }}/ --with-toolchain-path=${HOME}/sysroot-${{
>> matrix.debian-arch }}/ --with-freetype-lib=${HOME}/sysroot-${{
>> matrix.debian-arch }}/usr/lib/${{ matrix.gnu-arch }}-linux-gnu${{
>> matrix.gnu-flavor}}/ --with-freetype-include=${HOME}/sysroot-${{
>> matrix.debian-arch }}/usr/include/freetype2/
>> --x-libraries=${HOME}/sysroot-${{ matrix.debian-arch }}/usr/lib/${{
>> matrix.gnu-arch }}-linux-gnu${{ matrix.gnu-flavor}}/" >> $GITHUB_ENV
>
> No chance to break this line by option? It is kinda hard to read and thus
> verify. It is still okay if we cannot.
Sure, I agree, perhaps just putting that as a separate step would increase
readability.
-
PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Fri, 20 Nov 2020 17:35:12 GMT, Aleksey Shipilev wrote: >> Looks good! A few minor nits, if you want to address them. > > Ah wait, I now see "Linux additional" is the column name in testing table, > because it is the name of the job! Eh... It was nicer to have columns per > arch. Does it make sense to split the "Linux x64 (other)" and "Linux Foreign" > then? It might also simplify the coding as we would not have to check for > `matrix.debian_arch`. > > EDIT: Or maybe it is fine to have "Linux additional", as it stand now. Really > undecided here :) I personally think I prefer to just use a single column for the additional cross compile builds, because the table becomes very big otherwise (and the rows get split up). You still see the full name of the build in case anything fails, so not sure it adds that much value to get an additional box for each cross-compiled architecture, I guess in 99% of cases they will just pass and then you don't really care exactly which platforms you didn't break.. But I'm no UX expert. :) - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Fri, 20 Nov 2020 17:31:51 GMT, Aleksey Shipilev wrote: >> Robin Westberg has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve naming, fix style issues > > Looks good! A few minor nits, if you want to address them. Ah wait, I now see "Linux additional" is the column name in testing table, because it is the name of the job! Eh... It was nicer to have columns per arch. Does it make sense to split the "Linux x64 (other)" and "Linux Foreign" then? It might also simplify the coding as we would not have to check for `matrix.debian_arch`. EDIT: Or maybe it is fine to have "Linux additional", as it stand now. Really undecided here :) - PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Fri, 20 Nov 2020 16:09:59 GMT, Robin Westberg wrote:
>> .github/workflows/submit.yml line 469:
>>
>>> 467: name: transient_jdk-linux-x64_${{
>>> needs.prerequisites.outputs.bundle_id }}
>>> 468: path: ~/jdk-linux-x64
>>> 469: if: steps.build_restore.outcome == 'failure'
>>
>> I may be blind, but I don't see any step with id `build_restore` in new
>> file...
>
> It already existed in the step right above so I didn't have to add it.. This
> follows the same pattern as other artifact downloads (I've seen them fail a
> fair amount of time in the past).
Right. Got myself confused with GitHub UI.
-
PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Fri, 20 Nov 2020 16:14:19 GMT, Robin Westberg wrote:
>> Currently Linux x64 testing in GitHub Actions depends on a few non-relevant
>> hotspot build-only jobs (such as zero) that prevents testing from being run
>> if those build were to fail. As the tests only require the x64 release and
>> debug builds to run and provide interesting feedback, we should separate
>> these other builds into a different job.
>>
>> To reduce duplicated steps, all the existing hotspot-only builds have been
>> consolidated into a single job.
>
> Robin Westberg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve naming, fix style issues
Looks good! A few minor nits, if you want to address them.
.github/workflows/submit.yml line 526:
> 524: echo "CC=${{ matrix.gnu-arch }}-linux-gnu${{
> matrix.gnu-flavor}}-gcc-10" >> $GITHUB_ENV
> 525: echo "CXX=${{ matrix.gnu-arch }}-linux-gnu${{
> matrix.gnu-flavor}}-g++-10" >> $GITHUB_ENV
> 526: echo "cross_flags=--openjdk-target=${{ matrix.gnu-arch
> }}-linux-gnu${{ matrix.gnu-flavor}} --with-sysroot=${HOME}/sysroot-${{
> matrix.debian-arch }}/ --with-toolchain-path=${HOME}/sysroot-${{
> matrix.debian-arch }}/ --with-freetype-lib=${HOME}/sysroot-${{
> matrix.debian-arch }}/usr/lib/${{ matrix.gnu-arch }}-linux-gnu${{
> matrix.gnu-flavor}}/ --with-freetype-include=${HOME}/sysroot-${{
> matrix.debian-arch }}/usr/include/freetype2/
> --x-libraries=${HOME}/sysroot-${{ matrix.debian-arch }}/usr/lib/${{
> matrix.gnu-arch }}-linux-gnu${{ matrix.gnu-flavor}}/" >> $GITHUB_ENV
No chance to break this line by option? It is kinda hard to read and thus
verify. It is still okay if we cannot.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
On Fri, 20 Nov 2020 14:39:54 GMT, Aleksey Shipilev wrote:
>> Robin Westberg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve naming, fix style issues
>
> .github/workflows/submit.yml line 380:
>
>> 378: continue-on-error: true
>> 379:
>> 380: linux_x64_hotspot_only:
>
> This is a weird name for a job that also cross-compiles non-x64 versions.
> Maybe `linux_aux_builds` or some such?
Yeah this can probably be improved, how about the updated version?
> .github/workflows/submit.yml line 469:
>
>> 467: name: transient_jdk-linux-x64_${{
>> needs.prerequisites.outputs.bundle_id }}
>> 468: path: ~/jdk-linux-x64
>> 469: if: steps.build_restore.outcome == 'failure'
>
> I may be blind, but I don't see any step with id `build_restore` in new
> file...
It already existed in the step right above so I didn't have to add it.. This
follows the same pattern as other artifact downloads (I've seen them fail a
fair amount of time in the past).
> .github/workflows/submit.yml line 506:
>
>> 504: run: >
>> 505: sudo qemu-debootstrap
>> 506: --arch=${{ matrix.debian-arch}}
>
> Style: `${{ matrix.debian-arch }}` (space before closing braces) -- also in
> other places.
Ah right, good catch, fixed these.
> .github/workflows/submit.yml line 532:
>
>> 530: run: >
>> 531: bash configure
>> 532: --with-conf-name=linux-x64-hotspot
>
> That does not look a proper config name for cross-compiled jobs?
No you are right, I didn't really care since it's not visible anywhere, but can
certainly update it a bit.
-
PR: https://git.openjdk.java.net/jdk/pull/1350
Re: RFR: 8256747: GitHub Actions: decouple the hotspot build-only jobs from Linux x64 testing [v2]
> Currently Linux x64 testing in GitHub Actions depends on a few non-relevant > hotspot build-only jobs (such as zero) that prevents testing from being run > if those build were to fail. As the tests only require the x64 release and > debug builds to run and provide interesting feedback, we should separate > these other builds into a different job. > > To reduce duplicated steps, all the existing hotspot-only builds have been > consolidated into a single job. Robin Westberg has updated the pull request incrementally with one additional commit since the last revision: Improve naming, fix style issues - Changes: - all: https://git.openjdk.java.net/jdk/pull/1350/files - new: https://git.openjdk.java.net/jdk/pull/1350/files/9cf0c80d..c79e6ab5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1350=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1350=00-01 Stats: 21 lines in 1 file changed: 0 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/1350.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1350/head:pull/1350 PR: https://git.openjdk.java.net/jdk/pull/1350
