Skip to content

suppress warnings from remote dependencies #5605

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 18, 2022
Merged

suppress warnings from remote dependencies #5605

merged 1 commit into from
Jun 18, 2022

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Jun 18, 2022

motivation: warnings from remote dependencies are not actionable and make the use of -warnings-as-errors difficult

changes:

  • wire up the ResolvedPackage to SwiftTargetBuildDescription
  • pass -suppress-warnings for remote dependencies
  • add unit test

@tomerd
supress warnings from remote dependencie
a9462ca 
motivation: warnings from remote dependencies are not actionable and make the use of -warnings-as-errors difficult

changes:
* wire up the ResolvedPackage to SwiftTargetBuildDescription
* pass -suppress-warnings for remote dependencies
* add unit test

rdar://94473684
@tomerd tomerd changed the title supress warnings from remote dependencie suppress warnings from remote dependencie Jun 18, 2022
@tomerd
Copy link
Contributor Author

tomerd commented Jun 18, 2022

@swift-ci smoke test

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Jun 18, 2022
@tomerd tomerd changed the title suppress warnings from remote dependencie suppress warnings from remote dependencies Jun 18, 2022
ktoso
ktoso approved these changes Jun 18, 2022
View reviewed changes
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Very nice! Thank you :-)

Sources/Build/BuildPlan.swift
if self.package.isRemote {
args += ["-suppress-warnings"]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome 👏

abertelrud
abertelrud approved these changes Jun 18, 2022
View reviewed changes
Sources/Build/BuildPlan.swift
case .registry, .remoteSourceControl, .localSourceControl:
return true
case .root, .fileSystem:
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this would make path dependencies be considered local and not remote, right? If so, I completely agree, that's seems like the right semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly

@abertelrud
Copy link
Contributor

This is great!

@tomerd tomerd merged commit acaf214 into swiftlang:main Jun 18, 2022
tomerd added a commit that referenced this pull request Jun 18, 2022
@tomerd
supress warnings from remote dependencies (#5605)
ae228ce 
motivation: warnings from remote dependencies are not actionable and make the use of -warnings-as-errors difficult

changes:
* wire up the ResolvedPackage to SwiftTargetBuildDescription
* pass -suppress-warnings for remote dependencies
* add unit test

rdar://94473684
tomerd added a commit to tomerd/swift-package-manager that referenced this pull request Jun 18, 2022
@tomerd
supress warnings from remote dependencies (swiftlang#5605)
3d21280 
motivation: warnings from remote dependencies are not actionable and make the use of -warnings-as-errors difficult

changes:
* wire up the ResolvedPackage to SwiftTargetBuildDescription
* pass -suppress-warnings for remote dependencies
* add unit test

rdar://94473684
@tomerd tomerd mentioned this pull request Jun 18, 2022
Merged
tomerd added a commit that referenced this pull request Jun 18, 2022
@tomerd
supress warnings from remote dependencies (#5605) (#5608)
796b7ff
@MPLew-is
Copy link

This is fantastic to have, but it looks like remarks still made it through:

{redacted}/.build/checkouts/swift-aws-lambda-runtime/Sources/AWSLambdaRuntimeCore/LambdaContext.swift:18:17: remark: '@preconcurrency' attribute on module 'NIOCore' is unused
@preconcurrency import NIOCore
~~~~~~~~~~~~~~~~^

This won't limit usage of -warnings-as-errors, but it definitely still leads to clutter in the build output that you really can't do anything about. I don't see any compiler flags to silence remarks though - do you have any thoughts as to how we might go about silencing these for remote dependencies as well?

@tomerd
Copy link
Contributor Author

tomerd commented Sep 19, 2022

@elsh @abertelrud @neonichu @artemcm thoughts?

@MPLew-is
Copy link

@tomerd @elsh @abertelrud @neonichu @artemcm any other updates or thoughts on silencing remarks?

@artemcm
Copy link
Contributor

artemcm commented Oct 27, 2022

@tomerd @elsh @abertelrud @neonichu @artemcm any other updates or thoughts on silencing remarks?

That sounds like a very reasonable request to me.

@artemcm
Copy link
Contributor

artemcm commented Oct 27, 2022

I'm a bit surprised we have remarks that are emitted without being explicitly requested with some -R... flag, but if we are emitting these unprompted, then being able to silence them similarly to warnings makes sense.

@tomerd
Copy link
Contributor Author

tomerd commented Oct 27, 2022

@artemcm what is the way to silence them?

@artemcm
Copy link
Contributor

artemcm commented Oct 27, 2022

@artemcm what is the way to silence them?

We do not currently have a way to do that, we need to build one that's similar to -suppress-warnings, perhaps something like this:
swiftlang/swift#61762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

@abertelrud abertelrud abertelrud approved these changes

@neonichu neonichu Awaiting requested review from neonichu

@elsh elsh Awaiting requested review from elsh

Assignees
No one assigned
Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomerd @abertelrud @MPLew-is @artemcm @ktoso