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

[builtins][CMake] Replace custom target for lse_builtin symlinks #66936

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

petrhosek
Copy link
Member

Using custom target with BYPRODUCTS results in symlinks being recreated on every build (the build is always dirty). We could instead use custom commands to generate the symlinks and by adding the output to the list of sources, build system will ensure those are created as needed.

@mstorsjo
Copy link
Member

Thanks for this, I've also been annoyed by these being remade all the time. I don't really know the details about the cmake constructs at work here though, so I'll defer the review to @smeenai.

Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

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

Never mind, I looked at the whole file and understand this better now. We're still adding the generated files to the sources. I thought there might be issues doing that with non-Ninja generators, but https://cmake.org/cmake/help/latest/command/add_custom_command.html#examples-generating-files seems to explicitly bless it, so we should be good.

The only thing that stood out to me on that same page was:

Each source file may have at most one command specifying it as its main dependency.

Which we're violating. Dunno if it matters in practice, but we could just use DEPENDS instead?

Using custom target with BYPRODUCTS results in symlinks being recreated
on every build (the build is always dirty). We could instead use custom
commands to generate the symlinks and by adding the output to the list
of sources, build system will ensure those are created as needed.
@petrhosek
Copy link
Member Author

The only thing that stood out to me on that same page was:

Each source file may have at most one command specifying it as its main dependency.

Which we're violating. Dunno if it matters in practice, but we could just use DEPENDS instead?

Thanks for catching that, I missed this part. I changed MAIN_DEPENDENCY to DEPENDS.

@petrhosek petrhosek merged commit 4dec62f into llvm:main Sep 21, 2023
2 checks passed
@petrhosek petrhosek deleted the builtins-lse-build branch September 21, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants