-
Notifications
You must be signed in to change notification settings - Fork 81
TST: add test package depending on a cmake subproject #599
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
Conversation
47de165 to
b1f6a08
Compare
|
This is Draft until the upstream bugs are fixed, right? |
|
It depends on your definition of draft PR 🙂. The PR is complete, but there is a bug in Meson that makes merging it not productive. I keep this open just to remember ourselves of the bug: I don't know cmake at all and I don't have the spare cycles required to investigate this. |
|
👍🏼 In my book:
|
|
This was supposed to be non-draft and red, thus also not ready for review, probably. But it is so old that the test results have been flushed out. Setting it as draft now. |
183ec0f to
ece7218
Compare
ece7218 to
87db821
Compare
87db821 to
68e93fc
Compare
|
This test does not really test much on the meson-python side, but it was useful to identify the problem on the Meson side, therefore I'm thinking to merge it to avoid the support for CMake subprojects to regress in the future. @rgommers, what do you think? |
|
That seems like a good idea to me, covering this kind of thing with tests is helpful for Our CI doesn't check macOS + meson master, did you test that locally? If not I'll do that before merging. |
Yes, I tested that locally. On macOS the test passes also with older versions on Meson. The Meson fix is necessary only on Linux. However, I did not think that verifying this in CI was necessary. We can do that, if you think it is useful. |
ad5eca4 to
0b9d442
Compare
0b9d442 to
a978150
Compare
|
I extended the test to run on macOS too. Merging. |
No description provided.