Skip to content

Temporarily disable target-based dependency resolution #2998

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

Merged

Conversation

neonichu
Copy link
Contributor

We are seeing some issues with the full implementation of target-based dependency resolution done in #2749 and need to temporarily disable it to unblock affected projects. The changes are still available behind a compile-time define ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION and the tests have been modified accordingly to work in both cases (some tests are skipped by default for now and only enabled when enabling target-based dependency resolution).

I haven't yet distilled the issues into test cases, but once I have I'll start a branch of re-enabling this again.

rdar://problem/65284674

We are seeing some issues with the full implementation of target-based
dependency resolution done in swiftlang#2749 and need to temporarily disable it
to unblock affected projects. The changes are still available behind a
compile-time define `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` and the
tests have been modified accordingly to work in both cases (some tests
are skipped by default for now and only enabled when enabling
target-based dependency resolution).

I haven't yet distilled the issues into test cases, but once I have I'll
start a branch of re-enabling this again.
@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud
Copy link
Contributor

Ideally there would be a way to enable it dynamically, using a feature flag (I wonder why it wasn't behind a feature flag to begin with, actually). If that's a lot of code change then that might be a follow-on PR, but that seems that an approach that would allow testing to continue as the bugs are fixed.

@SDGGiesbrecht
Copy link
Contributor

I cannot see the radar. May I ask what the issues are?

@neonichu
Copy link
Contributor Author

@abertelrud I opted to not introduce a flag since this is hopefully a very temporarily change. If that ends up to not be the case, I'll convert it into a flag in a follow-on.

@neonichu
Copy link
Contributor Author

neonichu commented Oct 23, 2020

@SDGGiesbrecht Sorry, I haven't been able to distill the issues down into examples I can share publicly at this point, as mentioned in the PR description.

There's one example I can share, which is from the source-compatibility suite:

$ git clone  https://github.com/Bouke/SRP.git
$ cd SRP
$ git checkout b166838d4cf9218933c03151d62e50772460f95e
$ swift package resolve
...
error: the Package.resolved file is most likely severely out-of-date and is preventing correct resolution; delete the resolved file and try again

I'll share more examples as I have them.

@neonichu neonichu merged commit 27f444f into swiftlang:main Oct 23, 2020
@neonichu neonichu deleted the disable-target-based-dependency-resolution branch October 23, 2020 23:59
@SDGGiesbrecht
Copy link
Contributor

I’m not sure how to set it up to replicate that error. It won’t even load the manifest for me at the moment:

git clone https://github.com/Bouke/SRP
cd SRP
git checkout b166838d4cf9218933c03151d62e50772460f95e
cd ..
git clone https://github.com/apple/swift-package-manager
cd swift-package-manager
git checkout ae748dd8cb91307b9a10babe0d452bcc4c9034af
swift build
swift run swift-package resolve --package-path ../SRP
[...]/SRP: error: manifest parse error(s):
[...]/SRP/Package.swift:18:29: error: cannot convert value of type 'Int' to expected element type 'Array<SwiftVersion>.ArrayLiteralElement' (aka 'SwiftVersion')
    swiftLanguageVersions: [4]
                            ^

Looking back over the code, I’m going to take a wild guess and say that when I removed the filters from the pins in #2794 after the fact, this graph traversal possibly became invalid as something keyed by name, and needs special tailoring reminiscent of this other graph traversal. If that first traversal is faulty and capable of accidentally pruning part of the graph as it loads, I could imagine SwiftPM mistakenly concluding a pins file were at fault. But it is just a hunch.

@SDGGiesbrecht
Copy link
Contributor

Okay, I figured out where the other errors were coming from:

Self Hosting

Note: PackageDescription v4 is not available when developing using this method.

Now that I have wasted so much time trying to fix a problem that isn’t a problem, I can move on to the real debugging... 🤦‍♂️

@SDGGiesbrecht
Copy link
Contributor

@neonichu, I solved it. It was a simple fix. RepositoryPackageContainer’s cache was not parameterized by product filter, so the first filter resolved for any version was being incorrectly reused for every subsequent query. It never showed up in testing because most tests had RepositoryPackageContainer swapped out for a mock.

I’ll submit a pull request in a few minutes when I clean up my commit history.

@SDGGiesbrecht
Copy link
Contributor

I submitted #3006.

federicobucchi pushed a commit to federicobucchi/swift-package-manager that referenced this pull request Jan 6, 2021
We are seeing some issues with the full implementation of target-based
dependency resolution done in swiftlang#2749 and need to temporarily disable it
to unblock affected projects. The changes are still available behind a
compile-time define `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` and the
tests have been modified accordingly to work in both cases (some tests
are skipped by default for now and only enabled when enabling
target-based dependency resolution).

I haven't yet distilled the issues into test cases, but once I have I'll
start a branch of re-enabling this again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants