RFR: 8320931: [REDO] dsymutil command leaves around temporary directories [v2]

[Jaikiran Pai]

On Thu, 30 Nov 2023 09:39:54 GMT, Jaikiran Pai <jpai at 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