Skip to content

Add and fix missing Triple tests #6842

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 27, 2023
Merged

Add and fix missing Triple tests #6842

merged 5 commits into from
Sep 27, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Aug 23, 2023 •
edited
Loading

Vendored Swift Driver triple was not checked against a few tests that weren't brought over from TSC after TSC.Triple type was deprecated. We should fix those tests, especially as they verified that per-component equality for triples worked instead of the current string-based equality check.

Also fixed some of the Utilities/soundness.sh script failures.

Related to rdar://113967401

@MaxDesiatov MaxDesiatov self-assigned this Aug 23, 2023
@MaxDesiatov MaxDesiatov requested a review from rauhul August 23, 2023 14:36
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

MaxDesiatov added a commit that referenced this pull request Aug 23, 2023
@MaxDesiatov
Add and fix missing Triple tests, fix soundness checks
047a408 
Cherry-pick of #6842.

Vendored Swift Driver triple was not checked against a few tests that weren't brought over from TSC after `TSC.Triple` type was deprecated. We should fix those tests, especially as they verified that per-component equality for triples worked instead of the current string-based equality check.

Also fixed `Utilities/soundness.sh` script failures.

(cherry picked from commit 2cdc604)

```
# Conflicts:
#	Tests/BasicsTests/FileSystem/PathShimTests.swift
#	Tests/BasicsTests/FileSystem/PathTests.swift
```
@MaxDesiatov MaxDesiatov mentioned this pull request Aug 23, 2023
Merged
@neonichu
Copy link
Contributor

Thanks!

The changes to vendored files are only swiftformat related, right?

@MaxDesiatov
Copy link
Contributor Author

The changes to vendored files are only swiftformat related, right?

Not exclusively, let me revert the formatting changes to make it easier to review those.

@MaxDesiatov MaxDesiatov force-pushed the maxd/fix-triple-soundness branch from 2cdc604 to cfb7836 Compare August 23, 2023 15:36
MaxDesiatov
MaxDesiatov commented Aug 23, 2023
View reviewed changes
Sources/Basics/Vendor/Triple+Platforms.swift Outdated
@@ -239,53 +239,6 @@ extension Triple {
}
}

// The Darwin platform version used for linking.
public var darwinLinkerPlatformVersion: Version {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as unused and therefore not worth adapting to the optionality changes made in this PR.

MaxDesiatov
MaxDesiatov commented Aug 23, 2023
View reviewed changes
Sources/Basics/Vendor/Triple.swift Outdated
@@ -72,14 +72,16 @@ public struct Triple {

/// Represents a version that may be present in the target triple.
public struct Version: Equatable, Comparable, CustomStringConvertible {
public static let zero = Version(0, 0, 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as unused, previously it was incorrectly used as a substitute for nil where Triple.Version should've been optional in the first place.

@MaxDesiatov
Copy link
Contributor Author

@neonichu ready for review

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

3 similar comments
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@neonichu
Copy link
Contributor

Not exclusively, let me revert the formatting changes to make it easier to review those.

What's the plan here, are you planning to upstream the changes? It doesn't seem ideal to make permanent changes because that'll make it harder to update the vendored copy.

Another option could be to wrap the entire driver triple so that we control the API and don't have to care about any of its surface that we think is wrong.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test linux

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

I prefer to upstream these changes, will create corresponding PRs on swift-driver after this one is merged.

@neonichu
Copy link
Contributor

I prefer to upstream these changes, will create corresponding PRs on swift-driver after this one is merged.

Could we do that in reverse order for the main PR? I think only the 5.9 PR is urgent, so we can just wait on feedback from the driver team for the main one. Just trying to avoid having to change this again if there's feedback from them that materially changes what is happening in this PR.

@MaxDesiatov
Copy link
Contributor Author

Makes sense, I need a review on the 5.9 PR then so that it unblocks versioned triples checks in #6819. Thanks!

MaxDesiatov added a commit that referenced this pull request Aug 24, 2023
@MaxDesiatov
[5.9] Add and fix missing Triple tests, fix soundness checks (#6843)
3955272 
Cherry-pick of #6842.

Vendored Swift Driver triple was not checked against a few tests that weren't brought over from TSC after `TSC.Triple` type was deprecated. We should fix those tests, especially as they verified that per-component equality for triples worked instead of the current string-based equality check.

Also fixed some of the `Utilities/soundness.sh` script failures.

* Revert formatting changes

(cherry picked from commit cfb7836)

* Fix optionality issues after revert

(cherry picked from commit 8de70d3)
@MaxDesiatov MaxDesiatov marked this pull request as draft September 20, 2023 18:20
@MaxDesiatov MaxDesiatov force-pushed the maxd/fix-triple-soundness branch from 8de70d3 to 5bcb8b8 Compare September 20, 2023 18:21
@MaxDesiatov MaxDesiatov changed the title Add and fix missing Triple tests, fix soundness checks Add and fix missing Triple tests Sep 25, 2023
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov marked this pull request as ready for review September 25, 2023 15:53
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit ffdb50e into main Sep 27, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/fix-triple-soundness branch September 27, 2023 19:50
MaxDesiatov added a commit that referenced this pull request Sep 28, 2023
@MaxDesiatov
Add and fix missing Triple tests (#6842)
fcb1dc8 
Vendored Swift Driver triple was not checked against a few tests that weren't brought over from TSC after `TSC.Triple` type was deprecated. We should fix those tests, especially as they verified that per-component equality for triples worked instead of the current string-based equality check.

Also fixed some of the `Utilities/soundness.sh` script failures.

Related to rdar://113967401
MaxDesiatov added a commit that referenced this pull request Sep 28, 2023
@MaxDesiatov
Add and fix missing Triple tests (#6842)
f62dac9 
Vendored Swift Driver triple was not checked against a few tests that weren't brought over from TSC after `TSC.Triple` type was deprecated. We should fix those tests, especially as they verified that per-component equality for triples worked instead of the current string-based equality check.

Also fixed some of the `Utilities/soundness.sh` script failures.

Related to rdar://113967401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neonichu neonichu neonichu approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@tomerd tomerd Awaiting requested review from tomerd

@rauhul rauhul Awaiting requested review from rauhul

Assignees

@MaxDesiatov MaxDesiatov

Labels
bug cross-compilation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MaxDesiatov @neonichu