RFR: 8286089: Intermittent WebKit build failure on macOS in JavaScriptCore

[Lukasz Kostyra]

On Wed, 29 Mar 2023 10:46:05 GMT, Jay Bhaskar <jbhaskar at openjdk.org> wrote:

>> modules/javafx.web/src/main/native/Source/JavaScriptCore/CMakeLists.txt line 198:
>> 
>>> 196:         )
>>> 197:         if(copy_result)
>>> 198:             message(WARNING "Failed to copy ${_file} to ${JavaScriptCore_SCRIPTS_DIR}/${_script}: ${copy_output}")
>> 
>> Shouldn't `${copy_output}` at the end of this message be `${copy_result}`?
>> 
>> If not, I think it would be worth to print the `${copy_result}` variable regardless, in case there are any issues with the copy process.
>
> do you mean as 
> if(copy_result)
>             message(WARNING "Failed to copy ${_file} to ${JavaScriptCore_SCRIPTS_DIR}/${_script}: ${copy_output}")
> should be as
> 
> if(copy_result)
>             message(WARNING "${copy_result")

Actually, you can disregard this comment - I thought that `cmake -E copy_if_different ...` returns some meaningful error code (ex. one of errno variables) but I double-checked and it seems it returns 0 if succeeded and 1 if failed. Won't bring us any good to print it out, so it can stay as it is.

>> modules/javafx.web/src/main/native/Source/JavaScriptCore/CMakeLists.txt line 210:
>> 
>>> 208:     endif()
>>> 209:     endif() # end for port "Java"
>>> 210:     add_custom_command(
>> 
>> Suggestion: I feel this add_custom_command doesn't have to be called anymore because our loop did that job already. Maybe this old behavior should be fallen back to only when we're not fulfilling the PORT "Java" condition, or it should be removed completely.
>> 
>> So, in short:
>> 
>> if(PORT STREQUAL "Java")
>>   # code for 10-try loop
>> else() # else for port "Java"
>>   # old add_custom_command call
>> endif() # end for port "Java"
>> 
>> 
>> Or keep the code as is and remove this add_custom_command call. What do you think?
>
> I intentionally left the previous code unchanged. 
>     add_custom_command(
>         OUTPUT ${JavaScriptCore_SCRIPTS_DIR}/${_script}
>         MAIN_DEPENDENCY ${_file}
>         WORKING_DIRECTORY ${JavaScriptCore_DERIVED_SOURCES_DIR}
>         COMMAND ${CMAKE_COMMAND} -E copy_if_different ${_file} ${JavaScriptCore_SCRIPTS_DIR}/${_script}
>         VERBATIM)
> copy_if_different  will do nothing , if the changed code already did copy
> 
> incase of our changed code fail , the remaining add_custom_comand would prevent build issue.

Understood, thanks for the explanation!

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1073#discussion_r1151769585
PR Review Comment: https://git.openjdk.org/jfx/pull/1073#discussion_r1151769841