On Wed, 29 Mar 2023 10:46:05 GMT, Jay Bhaskar <jbhas...@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
