Skip to content

Migrate code to use Swift Regex #6598

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented May 22, 2023

  • Fix compile warnings
  • Migrate part of regex code to use Swift Regex

Motivation:

Migrate part of regex code to use Swift Regex. (Safer and Easier)

We should also consider to deprecate the TSCBasic.RegEx

Modifications:

Migrate part of the regex code to use Swift Regex

Result:

Safer, Easier and less code if we use Swift Regex.

Requirement/Dependency:

Bump minimal supported OS version to macOS 13

When WWDC 2023 is over and macOS 14 is introduced, the bump may be easier to accept.

@MaxDesiatov
Copy link
Contributor

Swift stdlib Regex APIs are supported starting with macOS 13.0, while SwiftPM needs to support macOS 12.0. I reckon this will need to wait until we can bump the macOS deployment version, for which I can't give an estimate right now I'm afraid.

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented May 22, 2023

Swift stdlib Regex APIs are supported starting with macOS 13.0, while SwiftPM needs to support macOS 12.0. I reckon this will need to wait until we can bump the macOS deployment version, for which I can't give an estimate right now I'm afraid.

The last time we bump the macOS deployment is by #6138 .

Why SwiftPM needs to support macOS 12.0? (Due to CI machine is currently not full on macOS 13 or something else?)

And I'd like to know is there a clear vision what range of OS version we should support on? (Like most Swift Package will only maintain support for at most 3 version eg. Swift 5.6 5.7 5.8)

Currently SwiftPM supports macOS 12 and macOS 13. And I believe when (macOS 13)+1 is released, maybe we can consider to drop macOS 12 support.

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented May 22, 2023

#6138 was merged very recently. I'm inclined to think that the next deployment target bump won't happen at least until new macOS is released and widely available (including CI), but I can't comment on future plans, versions, and schedules.

@compnerd
Copy link
Member

Windows support for Regex is questionable. There is no Swift in Swift yet due to bugs with SILOptimizer. We need to ensure that this doesn't regress Windows.

@neonichu
Copy link
Contributor

@Kyle-Ye Thanks, this is a good change, but we'll have to wait as Max and Saleem are saying. Would you prefer that we keep this as a draft until a later point or close it?

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented May 23, 2023

I'll make this into draft and waiting for the blocking problem to be fixed.

@Kyle-Ye Kyle-Ye marked this pull request as draft May 23, 2023 02:40
@tomerd tomerd added the next waiting for next merge window label Jun 12, 2023
@MaxDesiatov
Copy link
Contributor

@Kyle-Ye would you mind resolving conflicts on this one? Thanks!

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 5, 2024

Yeah. I see #7313 is finally merged. I'll update the PR later today.

@Kyle-Ye Kyle-Ye force-pushed the optimize/kyle/regex_fix branch 2 times, most recently from b24e0c2 to ec95df5 Compare February 5, 2024 14:00
@Kyle-Ye Kyle-Ye marked this pull request as ready for review February 5, 2024 14:02
@Kyle-Ye Kyle-Ye requested a review from bnbarham as a code owner February 5, 2024 14:02
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 5, 2024

@swift-ci please test

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 5, 2024

Windows support for Regex is questionable. There is no Swift in Swift yet due to bugs with SILOptimizer. We need to ensure that this doesn't regress Windows.

Would you mind sharing with us the latest status of Regex support on Windows? cc @compnerd

@Kyle-Ye Kyle-Ye force-pushed the optimize/kyle/regex_fix branch from ec95df5 to 1c38d02 Compare February 5, 2024 16:19
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 5, 2024

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov requested a review from compnerd February 5, 2024 16:39
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 5, 2024

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

After this one is merged we'll have to follow up on swiftlang/swift-tools-support-core#467 too.

@compnerd
Copy link
Member

compnerd commented Feb 5, 2024

Doug was splitting out the regex bits such that bootstrapping isn't required and only the swift-syntax bits are. This is untested, but I believe might be done? I would say that run with cross-repository testing with swiftlang/swift#71349 to get the tests to build and run. I believe that @bnbarham might try to make some time to help get this enabled by default.

@Kyle-Ye Kyle-Ye force-pushed the optimize/kyle/regex_fix branch from 1c38d02 to 8fd223e Compare February 25, 2025 03:23
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 25, 2025

@swift-ci please test

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 25, 2025

Doug was splitting out the regex bits such that bootstrapping isn't required and only the swift-syntax bits are. This is untested, but I believe might be done? I would say that run with cross-repository testing with swiftlang/swift#71349 to get the tests to build and run. I believe that @bnbarham might try to make some time to help get this enabled by default.

Is there any updates on this and will it block the PR here? @compnerd

@Kyle-Ye Kyle-Ye force-pushed the optimize/kyle/regex_fix branch from 8fd223e to 95300ee Compare February 25, 2025 05:14
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 25, 2025

@swift-ci please test

Copy link
Contributor Author

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

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

The only missing part is Netrc which has a RegexUtil enum to handle the regex logic. Since it is private, I think it's OK not to deal with it right now.

For this PR, let's focus on removing TSCBasic.RegEx only.

@compnerd
Copy link
Member

Doug was splitting out the regex bits such that bootstrapping isn't required and only the swift-syntax bits are. This is untested, but I believe might be done? I would say that run with cross-repository testing with swiftlang/swift#71349 to get the tests to build and run. I believe that @bnbarham might try to make some time to help get this enabled by default.

Is there any updates on this and will it block the PR here? @compnerd

Sadly, I think it will. I am unable to switch to the early-swift-driver on Windows due to the multitude of pending changes. I suspect that we will need to wait until after WWDC to make progress here.

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 25, 2025

@swift-ci please test windows

@Kyle-Ye Kyle-Ye force-pushed the optimize/kyle/regex_fix branch from 95300ee to 6d4b9f7 Compare February 27, 2025 09:10
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 27, 2025

@swift-ci plese test

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Feb 27, 2025

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next waiting for next merge window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants