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

Adopt manylinux fix from https://reviews.llvm.org/D111383 #357

Closed
wants to merge 1 commit into from

Conversation

silvasean
Copy link
Contributor

No description provided.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

Not sure what the CMake version requirements are here, but I had that conditional since MLIR's minimum is currently 3.13. Looks good to me either way.

@silvasean
Copy link
Contributor Author

Note to self: retry this after we update to an LLVM version that has pulled in Mike's change. Seems to not work at the moment and we suspect it is cmake getting confused by MLIR asking for Development vs Torch-MLIR asking for Development.Module.

@mikeurbach
Copy link
Contributor

mikeurbach commented Oct 11, 2021

FYI I think this build error may be related to both MLIR and the downstream project (Torch-MLIR in this case) re-using the same CMake variable. I added unset(_python_development_component) in MLIR on my new revision: https://reviews.llvm.org/D111585. Doing that and copying the same for CIRCT appears to work on my Ubuntu machine that failed previously. I'll let you know when the MLIR change is accepted and we can try again here.

EDIT: I think there might still be an issue here, unrelated to unset or not. I'm still seeing something like the failure in this PR when testing my revision if I search for Development.Module on my dev machine. What's strange is I don't see that when I search for Development.Module in a container set up for building manylinux wheels.

@mikeurbach
Copy link
Contributor

@silvasean I think this should be good to pick up on the next integration with upstream. The confusion I had previously was related to a configuration with several external projects calling find_package(Python3 ...), where some asked for Development, and some asked for Development.Module. If everything asks for Development.Module, there shouldn't be any issue. Here's the CIRCT PR: llvm/circt#1977.

@silvasean
Copy link
Contributor Author

silvasean commented Oct 29, 2021

@mikeurbach on chat I remember seeing some breakage related to this flying by, is this still the preferred pattern or is there a new recommended pattern?

@mikeurbach
Copy link
Contributor

I think the main thing is to be consistent with how upstream MLIR calls find_package, and @stellaraccident added a helper for that: https://github.com/llvm/llvm-project/blob/c2f2c6b103bf6c481f937cbb5a44c721d416a3fe/mlir/CMakeLists.txt#L110. If you just call that, it should work. Let me know if not and I can try to help.

@silvasean
Copy link
Contributor Author

Ok cool. Looks like Stella updated us. Will close this PR then.

@silvasean silvasean closed this Oct 29, 2021
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
* implement

Signed-off-by: chentong <chentong@us.ibm.com>

* refine

Signed-off-by: chentong <chentong@us.ibm.com>

* use tempdir

Signed-off-by: chentong <chentong@us.ibm.com>

* docs

Signed-off-by: chentong <chentong@us.ibm.com>
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