Stop passing -resource-dir to the Frontend if the flag wasn't passed in on the command-line
#1827
Conversation
| @@ -392,11 +392,13 @@ extension Driver { | |||
| try addPathArgument(.absolute(workingDirectory), to: &commandLine, remap: jobNeedPathRemap) | |||
| } | |||
|
|
|||
| if parsedOptions.hasArgument(.resourceDir) || kind == .scanDependencies { | |||
There was a problem hiding this comment.
I didn't pass it for scanDependencies jobs either initially, then a single test ExplicitModuleBuildTests.testExplicitLinkLibraries failed. It appears that a swift-frontend -scan-dependencies job alone does not find the resource-dir properly, so I set this flag for that kind of job for now.
There was a problem hiding this comment.
What was the failure? It's possible this job relies on being able to look up linked libraries relative to resource directory.
There was a problem hiding this comment.
What was the failure?
It complains that it didn't find a single library:
/home/finagolfin/swift-driver/Tests/SwiftDriverTests/ExplicitModuleBuildTests.swift:549: error: ExplicitModuleBuildTests.testExplicitLinkLibraries : XCTUnwrap failed: expected non-nil value of type "LinkLibraryInfo" -
The test, which you just added in #1622 last summer, looks in the Swift resource directory next to whatever compiler was used, as these swift-driver tests require a Swift toolchain to be in the PATH when they're run, and makes sure various parts of the stdlib are available. This currently works because this swift-driver queries the frontend with -print-target-info then passes that Swift -resource-dir to all subsequent jobs, like the -scan-dependencies frontend job that this test invokes.
However, this seems like a frontend bug that this swift-driver is covering up, as it is the frontend that finds the Swift resource-dir for all other jobs anyway. I will look into why it does not do so only for -scan-dependencies in the frontend and fix that first, just ran this through the CI to see if any other tests broke on other platforms and they didn't.
It's possible this job relies on being able to look up linked libraries relative to resource directory.
Yep, the only question is who should supply the path to that Swift resource directory? We cannot easily do it in the test because it is just picking up the Swift frontend and resource-dir from the PATH, ie it could be anywhere.
I see no reason why the swift-frontend -scan-dependencies job can't find it, whereas the frontend always easily finds it for all other jobs, so I will look into fixing this in the frontend before coming back to this pull. If I'm missing anything, let me know.
There was a problem hiding this comment.
OK, I ran some experiments and it appears the problem is not in the frontend at all, but in this repo when trying to scan dependencies in-process by using libswiftScan. If I disable that, as you can see here, the failing test passes on linux, but some other caching and explicit module tests that probably require that in-process scanning then fail. If I simply disable two tests instead, as shown in the latest version of this pull, then the rest of the macOS tests pass, while other scan-dependencies tests still fail on linux for the lack of a resource-dir.
@artemcm, I see two options here:
- Only disable this resource-dir flag for non-
scan-dependenciesjobs, as I did initially, until we figure out why that in-process scanning works differently. - Figure out why in-process scanning works differently now- my guess is that it's passed a different
PATHin its environment- and try to fix that too.
Let me know what you'd like to do: if you choose 2., let me know any ideas you have for the difference.
There was a problem hiding this comment.
Thank you for the investigation! It makes sense that this would be due to a difference with the in-process scanner. And your theory on the different PATH seems plausible. I am okay with passing resource-dir here to the scanner invocation only in the meantime, to make up for that difference. And I will see if I can discern making the in-process scanner potentially not need that.
|
@swift-ci please test |
|
@swift-ci please test linux |
|
@swift-ci please test linux |
|
@swift-ci please test macos |
|
@swift-ci please test linux |
|
@swift-ci please test macos |
|
@swift-ci please test linux |
|
@swift-ci please test macos |
|
@swift-ci please test linux |
|
@swift-ci please test linux |
|
@swift-ci please test macos |
… passed in on the command-line The original C++ Driver works this way and this flag is being repurposed in the future. Make an exception for `-scan-dependencies` jobs because of a remaining issue with in-process scanning that needs to be tracked down.
|
@swift-ci please test |
|
@artemcm, ready for review, passed CI and I ran it through a toolchain build and source compatibility run in the linked compiler pull above too. The linux toolchain built fine while the mac toolchain build failed when installing some unrelated lldb file. The source compatibility run shows as failed, but examining the results shows that everything passed, likely some CI misconfiguration. The Windows toolchain build failed because that compiler pull found a misconfiguration in the Windows CI, but since the Windows CI run on this repo takes hours and built much of the toolchain fine anyway, that's irrelevant. |
|
Ping @artemcm, I'd like to get this in before the branch. It passed the Windows CI, but as usual github simply didn't register that, so just waiting for your review before re-running the Windows portion, simply so that github can notice that it passes now. |
|
@swift-ci test Windows platform |
As noted in #1562, this swift-driver always passes a
-resource-dirto theswift-frontend, whereas the C++ Driver doesn't, unless one was explicitly passed in by the user. Given the recent push to deprecate this flag, try to only use it when needed.I tested this on linux locally and saw no test regressions, so push it through the full CI now.