-
Notifications
You must be signed in to change notification settings - Fork 447
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
[P4Testgen] Fix include paths #3997
Conversation
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.
A bit of a bandaid, but OK for now.
@vlstill Does this fix your problem? |
COMMAND ${CMAKE_COMMAND} -E create_symlink ${CMAKE_CURRENT_BINARY_DIR}/p4testgen ${CMAKE_BINARY_DIR}/p4testgen | ||
COMMAND ${CMAKE_COMMAND} -E create_symlink ${P4C_BINARY_DIR_PATH_REL}/p4include ${CMAKE_CURRENT_BINARY_DIR}/p4include |
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.
Interestingly, this does not work for me (maybe because I have p4c embedded in other project, although it worked the recent testgen changes...). The creates symlink leads to /p4include
, suggesting that ${P4C_BINARY_DIR_PATH_REL}
is not set. As far as I can see, the variable is set in ebfp
, bmv2
and p4test
backends, but not in p4tools
. It would probably need to be set in this file to work correctly.
@@ -114,7 +114,9 @@ target_link_libraries( | |||
add_custom_target( | |||
linkp4testgen | |||
# Add some convenience links for invoking p4testgen. | |||
# TODO: How to best handle includes? | |||
COMMAND ${CMAKE_COMMAND} -E create_symlink ${CMAKE_CURRENT_BINARY_DIR}/p4testgen ${CMAKE_BINARY_DIR}/p4testgen |
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 is not related to the original issue, but I've just noticed you are linking p4testgen
to top-level project, even in p4c
is embedded into a downstream project using it.
I would suggest symlinking p4testgen
into ${P4C_BINARY_DIR}
instead is a better choice (even though it can break some downstream workflow). This will leave the downstream projects to do the symlinking on their level if they so choose.
This is just a thought for future cleanup.
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.
Iirc in the old setup of bf-p4c it was more convenient to have the binary in the top-level. This is not needed anymore, we can change it.
e881a38
to
4612d3d
Compare
Fixes #3996.