Proposal: Replace OrderedSet with Set

[dmhts]

Hey @giginet,

First of all, I wanted to thank you for making such a great tool and for keeping working on it.

In the code snippets below you can see that allTargets is an OrderedSet which leads to issues when Scipio is repeatedly used over a big set of dependencies with the .storage(LocalCacheStorage(), ...) cache mode.

Scipio/Sources/ScipioKit/Producer/FrameworkProducer.swift

Lines 92 to 94 in f332eb4

	let allTargets = OrderedSet(buildProducts.compactMap { buildProduct -> CacheSystem.CacheTarget? in
	guard [.library, .binary].contains(buildProduct.target.type) else {

Scipio/Sources/ScipioKit/Producer/FrameworkProducer.swift

Line 119 in f332eb4

let targetsToBuild = allTargets.subtracting(cacheEnabledTargets)

Namely, it leads to a crash in the OrderedSet implementation due to a failing assertion:

https://github.com/apple/swift-collections/blob/c60d2f274d61550ca7a80b5306d23235c5f5f85b/Sources/OrderedCollections/OrderedSet/OrderedSet%2BInsertions.swift#L22-L25

I'm not sure if duplicates can be avoided in your implementation, but the simplest way to solve the issue would be to use an unordered Set instead of the ordered one. I can confirm that making that change solves the issue.

Should I open a PR to fix that or do you see any reasons to keep the set ordered?

[giginet]

Thank you for the reporting issue!

As I recall, order was more important than duplication. Some list represents the dependency tree, so order have to be kept to build in the correct order.

Of course, I'm welcome to open PR, or I'll reduce OrderSet usage as much as possible.

[omochi]

これ僕が実装した部分ですね。

ここがOrderedSetになっているのは、依存関係をグラフソートした結果を維持しているからです。
こうしないとビルド順が不適切になり問題が起きていたためです。

仮に全てをPIFで適切に定義できれば、どんな順番でビルドしても、xcodebuild側で依存解決するので動きますが、
その制約を導入するのは実装と保守が難しくなるでしょう。

一方、キャッシュのキーとして順序が不要というのはそうかもしれません。
対応するのであれば、ここの OrderedSet はそのままにしつつ、
キャッシュキーの構築をするところだけ Set にして並び順の情報を落とす、
という対応が望ましいでしょう。

---

This is the part I implemented.

The reason it’s an OrderedSet here is that it maintains the result of graph sorting the dependencies. Without this, the build order would be incorrect and issues would arise.

If everything could be properly defined in PIF, any build order would work because xcodebuild would resolve the dependencies. However, introducing such a constraint would make the implementation and maintenance difficult.

On the other hand, it might be true that the order is unnecessary for cache keys. If we’re to address this, the desirable approach would be to keep the OrderedSet as is, but convert it to an unordered Set only when constructing cache keys, thus removing the ordering information.

[dmhts]

Thanks for your answers. I’ve recently encountered another similar issue related to violations of the Hashable requirement, which also manifests only in debug builds. I have an idea of how to solve it. I’ll open a PR with a proposed solution once it’s done. If you don’t mind, we could discuss further details there.

[dmhts]

Closing for now, as I found that #131 fixes the issue of violating Hashable's requirements in debug builds for this and other places where operations on a set of hashable build products were involved. My guess is that it was somehow related to the implicitly generated products that broke the requirements. I'll keep an eye on it and reopen the issue if needed.