On Thu, 30 Nov 2023 09:39:54 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change will attempts to workaround an >> issue in dsymutil? >> >> The previous attempt to use `--reproducer Off` has shown that it fails to >> build on some other Xcode versions other than 14.3.1. Users have reported it >> to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The `--reproducer` >> option isn't being recognized on those versions. >> >> This current PR proposes to use an alternate workaround, one which just sets >> an environment variable that Xcode 14.3.1 recognizes and prevents the >> creation of the leftover `dsymutil-*` temporary directories. Since this new >> approach uses an environment variable to workaround the issue, the command >> itself shouldn't ideally run into any usage issues. Once we agree upon this >> change, before integrating, I'll ask for inputs from those who had run into >> build issues to verify this alternate approach doesn't cause problems. >> >> I have locally verified that this new approach continues to work and doesn't >> create those temporary directories. Additionally tier1, tier2, tier3 has >> completed successfully in our CI environment with this change. >> >> Magnus, Erik, I looked around to see if there's any convention in setting >> such environment variables that will be used when invoking external tools >> during the build. I didn't find any similar usage. Although the current >> change in this PR is working, if there's a different and better way to do >> this, please do let me know. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - check for the support of --reproducer option before using it for dsymutil > - merge latest from master branch > - 8320931: [REDO] dsymutil command leaves around temporary directories Hello Magnus, thank you for those inputs. Based on that I've now modified this PR to check if the `dsymutil` supports the `--reproducer` option and only if it supports it, I then use that option. I started off with using the `DSYMUTIL_REPRODUCER_PATH` environment variable usage and thought of checking the `dsymutil` version to decide whether or not to set that environment variable. However, it's extremely unclear which exact version of `dsymutil` should be considered as supporting it. In the upstream llvm project, the commit that introduced this auto generation of temporary directories (along with the `--reproducer` option) is this one https://github.com/llvm/llvm-project/commit/33b6891db24dd1e6702b4f04d2b08c1bf417dbee and going by the git tags associated with that commit, it would appear that even 15.0.x version of dsymutil would support the `--reproducer` option. However, we have had reports that `--reproducer` isn't supported on: Apple LLVM version 15.0.0 Optimized build. Default target: arm64-apple-darwin23.1.0 Host CPU: apple-m1 ``` I think we can't rely on the version numbers of dsymutil alone to decide whether or not to set the `DSYMUTIL_REPRODUCER_PATH` environment variable. So I decided to add a check in the build configuration to see if `--reproducer` option is supported by `dsymutil` and since we were checking for this option anyway, then I felt that when this option is supported, it makes sense to set `--reproducer Off` instead of the `DSYMUTIL_REPRODUCER_PATH` environment variable. I have tested this change locally with: dsymutil --version Apple LLVM version 14.0.0 (clang-1400.0.29.202) Optimized build. Default target: arm64-apple-darwin23.1.0 Host CPU: apple-a12 which doesn't support this option and with: Apple LLVM version 14.0.3 (clang-1403.0.22.14.1) Optimized build. Default target: arm64-apple-darwin23.1.0 Host CPU: apple-m1 which supports this option. I've verified that the build change correctly sets the `--reproducer Off` when this option is supported and the temporary directories don't get created. tier1, tier2 and tier3 testing with this change is currently in progress. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16876#issuecomment-1833422791
