-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Clean up warnings, fix handling of dep version changes #8232
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
Clean up warnings, fix handling of dep version changes #8232
Conversation
Assume dependent products from prebuilts are being used.
@@ -352,9 +358,11 @@ private func checkAllDependenciesAreUsed( | |||
let usedByPackage = productDependencies.contains { $0.name == product.name } | |||
// We check if any of the products of this dependency is guarded by a trait. | |||
let traitGuarded = traitGuardedProductDependencies.contains(product.name) | |||
// Consider prebuilts as used | |||
let prebuilt = prebuilts[dependency.identity]?.keys.contains(product.name) ?? false | |||
|
|||
// If the product is either used directly or guarded by a trait we consider it as used |
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.
Nit: this comment has to be updated as well.
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 just removed that comment. It's redundant.
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.
thought (possibly-blocking): It's great we are eliminating a warning, but would it be possible to add an automated test to ensure we do not regress, and to also gain confidence the changes fix the underlying issue?
That's what the change in PrebuiltsTest does. |
I'm going to lump this in with another bigger change coming to fix a serious bug. |
Found a bug where if you change swift syntax version in your manifest, we were not changing the use of prebuilts to match. This adds a scan to do so. Also requires that we add the version of the prebuilt to the workspace state.
I found a bug where if you change swift syntax version in your manifest, we were not changing the use of prebuilts to match. I have added a scan to do so. Also requires that we add the version of the prebuilt to the workspace state. |
Code has changed a lot so needs a new review.
I found a bug where if you change swift syntax version in your manifest, we were not changing the use of prebuilts to match. I have added a scan to do so. Also requires that we add the version of the prebuilt to the workspace state. Assume dependent products from prebuilts are being used to remove the warning for swift-syntax.
Assume dependent products from prebuilts are being used.