-
Notifications
You must be signed in to change notification settings - Fork 258
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 bug in projectAsLibrary test #5545
Fix bug in projectAsLibrary test #5545
Conversation
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.
Fix itself LGTM - checking my understanding, the problem was incorrectly thinking a dependency did not have compatible options, and therefore falling back to verifying the dependency when that wasn't the intent?
I'm a little worried this is a big foot gun for users to hit in the same way if so.
No. The problem was dat I've refactored this PR, so the resulting code is now simpler. |
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.
I do think we need another category of options rather than special-casing --library
, but this is a good improvement for now.
I'm also very worried about a test that apparently should fail but instead takes a long time and then passes! But I don't think leaving this test in this state is a good idea just to not lose track of that problem.
Description
How has this been tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.