On Tue, 29 Oct 2024 23:48:22 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix 32/64-bit confusion in comment in VirtualMachineImpl.c > > make/scripts/compare.sh line 79: > >> 77: >> 78: if [ "$OPENJDK_TARGET_OS" = "windows" ]; then >> 79: DIS_DIFF_FILTER="$SED -r \ > > This is now being defined for windows-aarch64 too, when it previously wasn't. > Is that intentional? No, it was not intentional, as in I forgot about the aarch64 version of Windows. With that said, I think it still might make sense to keep it this way. I don't think anyone has ever tried running the compare script on windows-aarch64; if they had, the lack of a filter at all would have made it basically unusable. This pattern is trying to hide 64-bit hex strings, and it is reasonable to assume it will work for aarch64 as well. If it doesn't, then its better to use this as a starting point for tweaking. Good catch, though! > make/scripts/compare.sh line 1457: > >> 1455: THIS_SEC_BIN="$THIS_SEC_DIR/sec-bin.zip" >> 1456: if [ "$OPENJDK_TARGET_OS" = "windows" ]; then >> 1457: JGSS_WINDOWS_BIN="jgss-windows-x64-bin.zip" > > This is now being defined for windows-aarch64 too, when it previously wasn't. > That seems wrong, > given the "x64" suffix. Well... this was broken on windows-aarch64 before, too, since then it would have looked for `jgss-windows-i586-bin.zip`. I'm going to leave this as it is. Obviously there is a lot more work needed to get the compare script running on windows-aarch64, and I seriously doubt anyone care about that platform enough to spend that time (Microsoft themselves seems to have all but abandoned the windows-aarch64 port...). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1822382796 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1822386222
