SwiftPackageIndex / SwiftPackageIndex-Server

The Swift Package Index is the place to find Swift packages!
https://swiftpackageindex.com
Apache License 2.0
551 stars 44 forks source link

Package build failing due to sample code #2798

Closed stephencelis closed 10 months ago

stephencelis commented 10 months ago

Please describe the bug

We maintain a few nested projects in our swift-composable-architecture package, and they tend to always target the latest/greatest to demo new features. Recently, we added a macro dependency to one of these sample apps, but now the main build is failing, even though these targets are outside the main package:

https://swiftpackageindex.com/builds/136FD6AC-6A5C-486C-A3BB-5BA52D99533B

We do have an spi.yml specifying which schemes should be built, but I assume that even the presence of the other targets in the workspace are getting in the way.

Is there a way to specify just the project file and not the workspace in the spi.yml? Or is there another configuration option I'm not thinking of?

stephencelis commented 10 months ago

I just locked all the Swift versions to 5.9 to see if it helps. Will close if the build succeeds and if you think this is the right fix!

(Hoping this doesn't affect the reported compatibility 😅)

finestructure commented 10 months ago

Was this perhaps the same issue around macro permissions we saw in swift-parsing recently? Your build link is outdated so I can't tell what platform this was for (it would only affect xcodebuild platforms, i.e. all except macOS SwiftPM and linux). We've deployed a fix for that.

Looking at the latest build result I think it's fine now?

CleanShot 2023-12-20 at 12 18 55@2x

Let me know and I can take a closer look and pull up the full logs.

stephencelis commented 10 months ago

@finestructure Seems that bumping to 5.9 fixed a bunch! I'm a lil surprised by the visionOS failure, but think our Package.resolved file regressed. Hoping that'll get fixed soon!

stephencelis commented 10 months ago

@finestructure Actually, looks like locking to 5.9 is now locking compatibility as well:

image

(visionOS is failing incidentally, but I think I figured out the fix)

The package definitely builds and works in 5.7+... though the badge still says 5.7-5.9, but I think it's probably cached?

finestructure commented 10 months ago

Oh, I misunderstood what you meant by "locking to 5.9". I thought you wanted it to be 5.9+ only (which should have puzzled me more than it did 😅).

I do believe the failures are actual failures. When building with Swift 5.8 I can see errors like

/Users/builder/builds/o86TiJKT/3/finestructure/swiftpackageindex-builder/spi-builder-workspace/Sources/swift-composable-architecture-benchmark/StoreSuite.swift:7:14: error: type 'Feature' does not conform to protocol 'Reducer'
  var store: StoreOf<Feature>!

in the logs which I can reproduce running locally both via

env DEVELOPER_DIR=/Applications/Xcode-14.3.1.app swift build

and in Xcode 14.3.1:

CleanShot 2023-12-21 at 10 55 55@2x

This is in Sources/swift-composable-architecture-benchmark - so not in the Examples.

I'll try older tags of TCA to see when this started failing.

finestructure commented 10 months ago

One thing I notice is that on each build, Package.resolved changes and is pulling in newer dependencies that are locked. Running with

env DEVELOPER_DIR=/Applications/Xcode-14.3.1.app swift build --disable-automatic-resolution

to prevent this I find that 1.3.0 is the last tag that builds successfully with Xcode 14.3.1/Swift 5.8.

stephencelis commented 10 months ago

Ah, those are internal benchmarks that I didn't realize would build for the given target. I'll look into scoping that now.

to prevent this I find that 1.3.0 is the last tag that builds successfully with Xcode 14.3.1/Swift 5.8.

I guess I'm a little confused then how SPI was reporting success for 5.7 as recently as TCA 1.5.5?

finestructure commented 10 months ago

That might be because I only tested with a similar command on my machine, not with the actual command that our builder uses. I'll try this again with 1.5.5 and the actual build command tomorrow just to confirm that this used to work!

finestructure commented 10 months ago

Just to confirm: the iOS/5.8 build passes for 1.5.5 with the actual build command:

CleanShot 2023-12-22 at 13 24 53@2x

For reference, the full build command is:

env DEVELOPER_DIR=/Applications/Xcode-14.3.1.app xcrun xcodebuild -IDEClonedSourcePackagesDirPathOverride=$PWD/checkout/.dependencies -skipPackagePluginValidation -derivedDataPath $PWD/checkout/.derivedData build -scheme ComposableArchitecture -destination generic/platform=ios

I'm sure it was the missing -scheme parameter in my quick test above that made the difference.

finestructure commented 10 months ago

Ok, looking at the diff 1.5.5...1.5.6 I think I see what happened. This change:

CleanShot 2023-12-22 at 13 30 47@2x

made the scheme config only apply for 5.9. Was there a reason you added these? I don't think you want any swift_version constraints in that file anymore (not even for the doc targets, 5.9 is now the default).

I'll propose a PR.

finestructure commented 10 months ago

Ok, this isn't the whole story. xcrun xcodebuild -resolvePackageDependencies also fails when running it with 5.8 against 1.5.6 and I need to investigate why.

finestructure commented 10 months ago

The error is

xcodebuild: error: Could not resolve package dependencies:
  product 'DependenciesMacros' required by package 'tic-tac-toe' target 'AuthenticationClient' not found in package 'swift-dependencies'.
finestructure commented 10 months ago

I'm seeing that same error when opening the project (on main at revision 6ed03a729ba5b17528ec6cea45b53ea472f281fb) in Xcode-14.3.1 (Swift 5.8).

CleanShot 2023-12-22 at 14 16 00@2x

finestructure commented 10 months ago

The toc-tac-toe target looks a bit different in setup than the other examples in that it doesn't have an embedded swift-composable-architecture package with a Package.swift and a Package@swift-5.9.swift manifest. There's just a tic-tac-toe package in it which looks structurally different.

The problem seems to be the .product(name: "DependenciesMacros", package: "swift-dependencies") dependency but I tried removing it (and the associated import DependenciesMacros) but it then fails where @DependencyClient is being used.

I'm not sure how to make this work. I get the sense that perhaps Tic-tac-toe should be excluded from building the same way the other targets seem to be.

FWIW, if I just remove the target from the xcworkspace the xcodebuild -resolvePackageDependencies command works. So my guess is applying whatever it is that makes the other target "disappear" should fix this.

Does that help @stephencelis ?

stephencelis commented 10 months ago

@finestructure The tic-tac-toe package is in the Examples/ directory, and is only there as a code sample. Our code samples all target 5.9 for simplicity and so that we can show off the latest/greatest features. We don't care to factor the Examples into SPI at all if possible. Is there a way to tell SPI to ignore the root xcworkspace (which is what brings the package and example projects together) and just build/run the package?

finestructure commented 10 months ago

@stephencelis So normally you'd do that via the scheme: parameter in the .spi.yml file, which you have, and this would fix the build.

However, what we also do, as part of the processing, is run xcodebuild -resolvePackageDependencies and that doesn't take this parameter. I'll need to investigate if we can skip running package resolution if a scheme has been provided, which might help us avoid this problem.

What's interesting though is that Tic-tac-toe seems to be the only example tripping us up here. Perhaps there's some config that could be applied?

Another idea would be to set up a new ignore: directive or something that removes Examples from the checkout before running any of these commands.

Maybe we wouldn't even need a directive for this - there's probably no harm simply running rm -rf Examples in all package checkouts... 🤔 I mean what could possibly go wrong, right? 😆

finestructure commented 10 months ago

Maybe we wouldn't even need a directive for this - there's probably no harm simply running rm -rf Examples in all package checkouts... 🤔 I mean what could possibly go wrong, right? 😆

In fact, let me try this. It's not a big change and in my local testing just now this would resolve the issue without further interventions (besides https://github.com/pointfreeco/swift-composable-architecture/pull/2679, that'll definitely need merging).

We can make it configurable later if we feel we need more control over this.

finestructure commented 10 months ago

That's deployed and re-run:

CleanShot 2023-12-23 at 17 36 24@2x

stephencelis commented 10 months ago

Thanks for the background and for looking into this!