Skip to content

[Tofino] Try to fix the dependency of mksizes in bf-asm CMake. #5221

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

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Apr 5, 2025

It can happen that the generation of uptr_sizes.h occurs before mksizes has been built.

@github-actions github-actions bot added the tofino Topics related to the Tofino switch and back end. label Apr 5, 2025
@fruffy fruffy requested a review from ChrisDodd April 6, 2025 20:09
Copy link
Contributor

@ChrisDodd ChrisDodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, though I must admit to not really understanding how cmake managers depedencies

add_custom_target(bfasm_yaml DEPENDS ${BFASM_GEN_DIR}/lex-yaml.c)
add_dependencies(bfasm_yaml bfasm_uptr)

set_source_files_properties(${BFASM_GEN_DIR}/lex-yaml.c PROPERTIES GENERATED TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should a similar line be added for uptr_sizes.h as that is also a generated file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple lines down they are actually added, will remove this line.

@fruffy
Copy link
Collaborator Author

fruffy commented Apr 7, 2025

Looks fine, though I must admit to not really understanding how cmake managers depedencies

Beats me, also seems to change based on the CMake version. Just trying to make it explicit as possible that mksizes needs to be generated before the other utilities use it. This version seems to consistently work.

@fruffy fruffy force-pushed the fruffy/mksizes-cmake branch from 1e4afe5 to aa0c174 Compare April 7, 2025 07:39
@fruffy fruffy added this pull request to the merge queue Apr 7, 2025
Merged via the queue into main with commit 77cb959 Apr 7, 2025
20 checks passed
@fruffy fruffy deleted the fruffy/mksizes-cmake branch April 7, 2025 11:30
blackdragoon26 pushed a commit to blackdragoon26/p4c that referenced this pull request Apr 10, 2025
blackdragoon26 pushed a commit to blackdragoon26/p4c that referenced this pull request Apr 10, 2025
blackdragoon26 pushed a commit to blackdragoon26/p4c that referenced this pull request Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tofino Topics related to the Tofino switch and back end.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants