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

[CMake] Make cmake config more reliable? #5145

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

maerhart
Copy link
Member

@maerhart maerhart commented May 6, 2023

I've heard from someone who ported CIRCT main to work with mlir-16 that the LLVM cmake checks complain that there are cpp files not included in any library. I'm not sure if that's caused by changes in the LLVM cmake config since then or if generally all cpp files in that directory should be marked optional when there are multiple libraries declared in a CMakeLists.txt.

Could someone with more CMake knowledge take a look if that change makes sense?

@darthscsi
Copy link
Contributor

This looks ok to me, but I don't know the CMake issues involved. If this doesn't change the build, I don't see a reason not to make this change.

Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

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

I don't see a reason not to do this.

Though I don't know what the optional sources are for in the first place.

@maerhart
Copy link
Member Author

Thanks @darthscsi for taking a look!
I think the optional sources are just here to tell the LLVM CMake checkers that it shouldn't check that the file is used somewhere. Apparently this is always needed when there are multiple libraries defined in one CMakeLists file.

@maerhart maerhart merged commit db3e226 into main Jun 21, 2023
@maerhart maerhart deleted the dev/maerhart/reduce-more-reliable-cmake branch June 21, 2023 11:29
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.

2 participants