improve and simplify validation of target based dependencies #3280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tomerd
requested review from
abertelrud,
friedbunny and
neonichu
as code owners
|
@swift-ci please smoke test |
|
macOS CI broken due to swift-driver cc @artemcm |
tomerd
commented
Feb 16, 2021
| @@ -1255,4 +1263,551 @@ class PackageGraphTests: XCTestCase { | |||
| let store = try PinsStore(pinsFile: AbsolutePath("/pins"), fileSystem: fs) | |||
| XCTAssertThrows(StringError("duplicated entry for package \"Yams\""), { try store.restore(from: json) }) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@neonichu @abertelrud would love to get your review on the different permutations captured in these new tests 😅 and hear your opinion on:
- the diagnostics make sense and are better than before, I think they are!
- the backward compatibility 5.2 < 5.4 looks reasonable - we allow users to use dependency::name but no longer encourage it
- do we need to capture more permutations?
tomerd
changed the title
|
@swift-ci please smoke test macOS |
tomerd
added
the
ready
neonichu
reviewed
Feb 24, 2021
| DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in | ||
| result.check( | ||
| diagnostic: """ | ||
| product 'Unknown' not found. it is required by package 'Foo' target 'Foo'. |
There was a problem hiding this comment.
Not specific to this test, but does this kind of message now become ambiguous? It feels like conceptually we could now be referring to Package.name or to the last path component of the URL since we are treating them equally, even when they differ. I wonder if for remote packages, we should instead emit the URL here.
|
In terms of test coverage, I think we should also cover cases where there are multiple conflicting options:
I think none of these care covered, yet, but I may have missed something. There are a lot of tests which is great 👍 |
neonichu
approved these changes
Feb 25, 2021
motivation: SwiftPM 5.2 introduced target based dependencies which added complexity at code level and to users, this PR tries to both simplify the "rules" and the code changes: * allow target dependency in the form of ".product(name:, package:)" where package parameter is the last segment of the dependecy URL, which is the most intuitive choice * change validation code to encourage the above from instead of encouraging adding "name" attibute to the dependency decleration, as we want to get away from adding this attribute in the long run * add several tests that capture the numerous permutations we are coding for
tomerd
force-pushed
the
radar/65048461
branch
from
a7f7ced
to
58dbc24
Compare
|
@swift-ci please smoke test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
motivation: SwiftPM 5.2 introduced target based dependencies which added complexity at code level and to users, this PR tries to both simplify the "rules" and the code
changes:
rdar://65048461