Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Build] Remove build time dependency on Python to copy header files into build include directory. #615

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

phniix
Copy link
Contributor

@phniix phniix commented Sep 12, 2018

Removes the specific build time dependency on 'copyHeaderForBuild.py' by moving the script's logic into CMake.

This change moves the operations normally performed by the script from compile time to build configuration generation time.

Description of Change(s)

  • Removes copyHeaderForBuild.py.
  • Move logic that existed in copyHeaderForBuild.py into CMake.
  • I have my editors configured to strip needless trailing whitespace, so that appears in the changes too (soz for the extra noise in the PR!)

Fixes Issue(s)

@meshula
Copy link
Member

meshula commented Sep 12, 2018

It seems like a good idea to use cmake to do the copying rather than python, but wouldn't it make sense to do it with a custom command? Otherwise it won't copy automatically when the files are modified, only when cmake generate is called. Something along the lines of

add_custom_command(TARGET ${PROJECT_NAME} 
                    COMMAND ${CMAKE_COMMAND} -E copy_if_different
                    ${infile} ${outfile})

might be the way to go?

Similarly, there's

COMMAND ${CMAKE_COMMAND} -E make_directory 

to make the destination directory before copying the files.

@jtran56
Copy link

jtran56 commented Sep 14, 2018

Filed as internal issue #USD-4736.

@phniix
Copy link
Contributor Author

phniix commented Sep 14, 2018

@meshula it totally makes sense from a developer perspective to use the custom command to get that automation. From a build system perspective, the file copy at cmake time makes a little bit more sense. Getting that automation for developers would likely outweigh streamlining CI (but if it becomes an issue that people bring up, we could always add a flag that switches things around for more optimal builds).

I'm right behind you on keeping the add_custom_command().. and I think most developers would want to keep it as well!

We should retain prepending the preprocessor directive, #line 1 "path/to/source/header_file.h" to each copied header file. This makes it more complicated than using ${CMAKE_COMMAND} -E copy_if_different :(

We can use the form ${CMAKE_COMMAND} -P {script_name}, to have cmake execute commands from a script. This way, we will be able to replicate the functionality as cross-platform cmake, removing the need for Python.. but we still have to maintain the logic in an external file (it would be nice if cmake allowed functions like file() to follow COMMAND).

This would then morph this approach into something akin to:

Private.cmake

...snip...
add_custom_command(
                OUTPUT ${outfile}
                COMMAND
                    "${CMAKE_COMMAND}" -Dinfile="${infile}" -Doutfile"${outfile}" -Dheader_dest_dir="${header_dest_dir}" -P "${PROJECT_SOURCE_DIR}/cmake/macros/copyHeaderForBuild.cmake"
                MAIN_DEPENDENCY "${infile}"
                COMMENT "Copying ${f} ..."
                VERBATIM
                )
...snip...

copyHeaderForBuild.cmake

file(TO_NATIVE_PATH  ${header_dest_dir} DSTDIRVAR)
file(TO_NATIVE_PATH  ${infile} INFILE)
file(TO_NATIVE_PATH  ${outfile} OUTFILE)

if(NOT EXISTS "${DSTDIRVAR}")
    file(MAKE_DIRECTORY "${DSTDIRVAR}")
    if(NOT EXISTS ${header_dest_dir})
        get_filename_component(file_basename "${OUTFILE}" NAME)
            message(FATAL_ERROR
                           "Destination directory ${DSTDIRVAR} was not "
                           "created for ${file_basename}")
    endif()
endif()
file(READ "${INFILE}" _tmp_file_content)
file(WRITE "${OUTFILE}" "\#line 1 ${infile}\n")
file(APPEND "${OUTFILE}" "${_tmp_file_content}")

There's the important error checking that's going on in the existing Python file.. we could forgo that and deal with whatever cmake errors out in its place.

The way that you have captured TARGET in your rendition is bang on.

Private.cmake

...snip...
add_custom_command(TARGET ${LIBRARY_NAME} PRE_BUILD 
                COMMAND ${CMAKE_COMMAND} -E make_directory ${header_dest_dir} 
                COMMAND ${CMAKE_COMMAND} -Dinfile="${infile}" -Doutfile"${outfile}" -P "${PROJECT_SOURCE_DIR}/cmake/macros/copyHeaderForBuild.cmake"
                MAIN_DEPENDENCY "${infile}"
                COMMENT "Copying ${f} ..."
                VERBATIM
                )
...snip...

copyHeaderForBuild.cmake

file(TO_NATIVE_PATH  ${infile} INFILE)
file(TO_NATIVE_PATH  ${outfile} OUTFILE)

file(READ "${INFILE}" _tmp_file_content)
file(WRITE "${OUTFILE}" "\#line 1 ${infile}\n")
file(APPEND "${OUTFILE}" "${_tmp_file_content}")

That is of course if we want to keep the #line directive.. which I feel is quite helpful given the file layout. If not, as you suggest, COMMAND ${CMAKE_COMMAND} -E copy_if_different, would be where I'd put my total support behind.

Removes build time dependency on Python script, 'copyHeaderForBuild.py' by moving the script's logic into a CMake script.

Maintains add_custom_command().
Invokes CMake script as part of the custom_command.
Moves directory creation into the custom_command.
@phniix
Copy link
Contributor Author

phniix commented Sep 15, 2018

A small push with some updates to get across what we have discussed thus far.
In practise, the TARGET need not be specified as a custom target, as ${LIBRARY_NAME}_headerfiles is made a dependency of ${LIBRARY_NAME} further along in the build script.

@pixar-oss pixar-oss merged commit da499f1 into PixarAnimationStudios:dev Nov 21, 2018
pixar-oss added a commit that referenced this pull request Nov 21, 2018
[Build] Remove build time dependency on Python to copy header files into build `include` directory.

(Internal change: 1915192)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants