-
Notifications
You must be signed in to change notification settings - Fork 397
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
CMake: Add initial support for ddrgen #3308
Conversation
Most if not all of the core logic should be in place. Just need to add documentation and squash commits |
@charliegracie @keithc-ca Can you review this PR? |
@genie-omr build all |
1 similar comment
@genie-omr build all |
cmake/modules/OmrDDRSupport.cmake
Outdated
set(DDR_BIN_DIR "${CMAKE_CURRENT_BINARY_DIR}/${DDR_TARGET_NAME}") | ||
set(DDR_TARGETS_LIST "${DDR_BIN_DIR}/targets.list") | ||
set(DDR_MACRO_INPUTS_FILE "${DDR_BIN_DIR}/macros.list") | ||
set(DDR_TOOLS_EXPORT "${omr_BINARY_DIR}/ddr/tools/DDRTools.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does DDRTools.cmake
come from?
nit: inconsistent indentation
TARGET "${target}" | ||
SOURCE_DIR "${OPT_SOURCE_DIR}" | ||
OUTPUT_ANNOTATED project_annt_hdr | ||
#TODO need to add pre-include files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just fix header files to no 'pre-include' is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do that for any of our code, however I would like to keep this in as a convenience for any downstream users.
@@ -21,3 +21,5 @@ | |||
|
|||
add_subdirectory(blob_reader) | |||
add_subdirectory(ddrgen) | |||
|
|||
export(TARGETS omr_blob_reader omr_ddrgen FILE "DDRTools.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does DDRTools.cmake
come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement generates the DDRTools.cmake
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get it; export
declares that this project produces DDRTools.cmake
- my question is how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cmake.org/cmake/help/v3.12/command/export.html
Export targets from the build tree for use by outside projects.
Create a filefilename
that may be included by outside projects to import targets from the current project’s build tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me rephrase: what defines the content of that file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its internally defined by cmake. When you include it it will define an imported target with the appropriate paths, etc. Here is a sample from when we export hookgen/tracegen
https://gist.github.com/dnakamura/24ecce3061474162e890f72838551619
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly cmake doesn't know anything about hookgen or tracegen: where is the information that tells it what to put in that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe it just puts the location where the targets are built
@keithc-ca Can you re-review this? It looks like @dnakamura pushed a new version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also a number of unanswered questions from my last review.
@@ -21,3 +21,5 @@ | |||
|
|||
add_subdirectory(blob_reader) | |||
add_subdirectory(ddrgen) | |||
|
|||
export(TARGETS omr_blob_reader omr_ddrgen FILE "DDRTools.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get it; export
declares that this project produces DDRTools.cmake
- my question is how?
6695556
to
bbe9052
Compare
Oops. appears i forgot to push last round of changes. @keithc-ca I believe i got them all |
@keithc-ca poke |
@genie-omr build all |
Arm failed due to infrastructure issues, while linux x86-64 and zos 390-64 failed with:
|
Signed-off-by: Devin Nakamura <[email protected]>
This is not implemented in cmake 3.4. Instead create omr_join utility function with the same behaviour Signed-off-by: Devin Nakamura <[email protected]>
Specifying this option forces the targets sources to be evaluated immediately, rather than at generate time. This is required to filter out use of $<TARGET_OBJECTS:>, which is not allowed on older versions of cmake Signed-off-by: Devin Nakamura <[email protected]>
@genie-omr build all |
No description provided.