Skip to content

Improve version-specific manifest selection #6801

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 1 commit into from
Aug 22, 2023
Merged

Improve version-specific manifest selection #6801

merged 1 commit into from
Aug 22, 2023

Conversation

@neonichu
Copy link
Contributor

@neonichu neonichu commented Aug 11, 2023

Currently, we only pick a non-exact match for version-specific manifests if its tools-version is higher than the regular one. That seems like an edge case only and we should also pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all.

@neonichu neonichu self-assigned this Aug 11, 2023
@neonichu
Copy link
Contributor Author

neonichu commented Aug 11, 2023

Needs a test for the new behavior.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 11, 2023

@swift-ci please smoke test

neonichu
neonichu commented Aug 11, 2023
View reviewed changes
Sources/PackageLoading/ToolsVersionParser.swift
return regularManifest
} else {
// If that's incompatible, use the closest version-specific manifest we got.
return versionSpecificManifest
Copy link
Contributor Author

@neonichu neonichu Aug 11, 2023

Choose a reason for hiding this comment

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

This is the key change, previously we would have always returned the regular manifest here, even if it was incompatible.

Copy link
Contributor

@abertelrud abertelrud Aug 11, 2023

Choose a reason for hiding this comment

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

Makes sense to me. And we wound't even be in this part of the code unless we knew we had a versionSpecificManifest, right, since the guard above would have returned the regular manifest previously?

This logic makes sense to me.

abertelrud
abertelrud reviewed Aug 11, 2023
View reviewed changes
abertelrud
abertelrud reviewed Aug 11, 2023
View reviewed changes
@neonichu
Copy link
Contributor Author

neonichu commented Aug 11, 2023

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Aug 11, 2023

@swift-ci please smoke test windows

@neonichu @abertelrud
Improve version-specific manifest selection
088b7d5 
Currently, we only pick a non-exact match for version-specific manifests if its tools-version is *higher* than the regular one. That seems like an edge case only and we should *also* pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all.

Co-authored-by: Anders Bertelrud <[email protected]>
@neonichu neonichu force-pushed the fix-version-specific-manifest-selection branch from 01b9271 to 088b7d5 Compare August 21, 2023 20:52
@neonichu
Copy link
Contributor Author

neonichu commented Aug 21, 2023

Added a test now.

@neonichu
Copy link
Contributor Author

neonichu commented Aug 21, 2023

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Aug 21, 2023

@swift-ci please smoke test windows

1 similar comment
@neonichu
Copy link
Contributor Author

neonichu commented Aug 21, 2023

@swift-ci please smoke test windows

@neonichu
Copy link
Contributor Author

neonichu commented Aug 21, 2023

¯\_(ツ)_/¯

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Aug 21, 2023

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Aug 21, 2023

@swift-ci please smoke test windows

for Windows it's just test windows for some reason

@neonichu neonichu enabled auto-merge (squash) August 21, 2023 21:33
@neonichu
Copy link
Contributor Author

neonichu commented Aug 22, 2023

Hitting the good old hang on Windows:

1338/1934 Test #1338: TestFoundation.TestProcess-test_run ..........................................................................................   Passed    0.16 sec
          Start 1339: TestFoundation.TestProcess-test_preStartEndState
Build timed out (after 60 minutes). Marking the build as aborted.
Build was aborted

@neonichu
Copy link
Contributor Author

neonichu commented Aug 22, 2023

@swift-ci test windows

@neonichu neonichu merged commit 667a006 into main Aug 22, 2023
@neonichu neonichu deleted the fix-version-specific-manifest-selection branch August 22, 2023 19:55
neonichu added a commit that referenced this pull request Aug 28, 2023
@neonichu @abertelrud
Improve version-specific manifest selection (#6801)
4645303 
Currently, we only pick a non-exact match for version-specific manifests if its tools-version is *higher* than the regular one. That seems like an edge case only and we should *also* pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all.

Co-authored-by: Anders Bertelrud <[email protected]>
neonichu added a commit that referenced this pull request Aug 28, 2023
@neonichu @abertelrud
Improve version-specific manifest selection (#6801)
54a13d7 
Currently, we only pick a non-exact match for version-specific manifests if its tools-version is *higher* than the regular one. That seems like an edge case only and we should *also* pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all.

Co-authored-by: Anders Bertelrud <[email protected]>
neonichu added a commit that referenced this pull request Aug 28, 2023
@neonichu @abertelrud
Improve version-specific manifest selection (#6801)
c1b84da 
Currently, we only pick a non-exact match for version-specific manifests if its tools-version is *higher* than the regular one. That seems like an edge case only and we should *also* pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all.

Co-authored-by: Anders Bertelrud <[email protected]>
neonichu added a commit that referenced this pull request Aug 28, 2023
@neonichu @abertelrud
Improve version-specific manifest selection (#6801)
8a0bafc 
Currently, we only pick a non-exact match for version-specific manifests if its tools-version is *higher* than the regular one. That seems like an edge case only and we should *also* pick it if the regular manifest is incompatible. This both matches intuition as well as our current documentation on the topic. It should also be backwards-compatible with the current behavior since the new behavior only comes into play if we would have previously failed to load a manifest at all.

Co-authored-by: Anders Bertelrud <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abertelrud abertelrud abertelrud left review comments

@MaxDesiatov MaxDesiatov Awaiting requested review from MaxDesiatov

+1 more reviewer

@tomerd tomerd tomerd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@neonichu neonichu

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@neonichu @MaxDesiatov @tomerd @abertelrud