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
Fix test for pybind11 include on conda #43
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@conda-forge-admin, please rerender |
The problem is that this installs the header folder to |
…nda-forge-pinning 2020.01.10
Thank you for the report! Do you see that issue only in local builds or also on CF? Cannot see an issue on Refs.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do NOT check only for file existence, that will always be true and is not what these two tests try to verify.
The modified test case checks that the files are installed where we intend them to be placed, according to conda-forge conventions. (We can additionally check if they exist.)
No, it was not true, this is why the PR failed until the patch was applied. The expected behavior is that It's a separate issue if that include directory is not the directory that is expected on conda that will also need fixing. I believe that this is being set by pybind11-feedstock/recipe/build.sh Line 7 in abbc6f7
But only for unix (it's not in bld.bat), so I think on unix systems this is going into the nested folder. We can test this too, but we need to know whether it's supposed to be |
(I can test this out locally later and will update if we need to change the include location) |
Okay, I think the actual patch we need for CF is to avoid writing the nested (sub)folder in the first place. |
Thank you again for your spotting and reporting this! |
44 and this PR are identical, by the way. Only change is I forgot to delete the unused patch. It's okay, though. |
Cheese, I did not see that you also adjusted Yes, this commit is the central one: f649a31 |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)This fixes the tests to make sure that the pybind11 include exists, rather than just assuming the base folder is the correct one. I expect this to fail, since currently the conda package does report the correct include directory.