Skip to content

Bump Swift version #4

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

Merged
merged 5 commits into from
Nov 2, 2021

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Oct 17, 2021

Summary

Bump the Swift version Package.swift use to 5.4. And add a new 5.5 Package.swift file

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Ran the ./bin/test script and it succeeded

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 17, 2021

Also upgrade the Swift version to make Markdown module to be able to use DocC for documentation

@QuietMisdreavus QuietMisdreavus self-requested a review October 18, 2021 23:18
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 27, 2021

Any idea on this PR @QuietMisdreavus ❤️

@Kyle-Ye Kyle-Ye force-pushed the feature/dependence_version branch from 0872f7e to bb40728 Compare October 27, 2021 12:18
@Kyle-Ye Kyle-Ye mentioned this pull request Oct 27, 2021
1 task
@Kyle-Ye Kyle-Ye force-pushed the feature/dependence_version branch from bb40728 to 27ff74c Compare October 27, 2021 12:49
@Kyle-Ye Kyle-Ye requested a review from franklinsch October 27, 2021 12:49
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 28, 2021

Done, can we run tests on this PR?

@Kyle-Ye Kyle-Ye force-pushed the feature/dependence_version branch 2 times, most recently from 9504cbb to 8fdd794 Compare October 28, 2021 12:04
@Kyle-Ye Kyle-Ye force-pushed the feature/dependence_version branch from 8fdd794 to 2853d7d Compare October 28, 2021 12:05
@franklinsch
Copy link
Contributor

@swift-ci please test

@franklinsch
Copy link
Contributor

When I delete the [email protected] file to force my toolchain to use Package.swift, I'm running into compiler errors I don't see on main, for example:

swift-markdown/Tests/MarkdownTests/Visitors/MarkupRewriterTests.swift:15:75: error: type 'Bundle' has no member 'module'
let everythingDocument = Document(parsing: try! String(contentsOf: Bundle.module.url(forResource: "Everything", withExtension: "md")!))
                                                                   ~~~~~~ ^~~~~~

@Kyle-Ye Kyle-Ye force-pushed the feature/dependence_version branch from 9cbd13d to 2853d7d Compare October 28, 2021 13:35
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 28, 2021

When I delete the [email protected] file to force my toolchain to use Package.swift, I'm running into compiler errors I don't see on main, for example:

swift-markdown/Tests/MarkdownTests/Visitors/MarkupRewriterTests.swift:15:75: error: type 'Bundle' has no member 'module'
let everythingDocument = Document(parsing: try! String(contentsOf: Bundle.module.url(forResource: "Everything", withExtension: "md")!))
                                                                   ~~~~~~ ^~~~~~

It seems that Bundle.module is introduces on Swift 5.3 which is auto-generated for getting Package Resource.(You can check the file by recovering [email protected] and Jump to Definition of module)

// Autogenerated: resource_bundle_accessor.swift
import class Foundation.Bundle

private class BundleFinder {}

extension Foundation.Bundle {
    /// Returns the resource bundle associated with the current Swift module.
    static var module: Bundle = {
        let bundleName = "swift-markdown_MarkdownTests"

        let candidates = [
            // Bundle should be present here when the package is linked into an App.
            Bundle.main.resourceURL,

            // Bundle should be present here when the package is linked into a framework.
            Bundle(for: BundleFinder.self).resourceURL,

            // For command-line tools.
            Bundle.main.bundleURL,
        ]

        for candidate in candidates {
            let bundlePath = candidate?.appendingPathComponent(bundleName + ".bundle")
            if let bundle = bundlePath.flatMap(Bundle.init(url:)) {
                return bundle
            }
        }
        fatalError("unable to find bundle named swift-markdown_MarkdownTests")
    }()
}

See https://developer.apple.com/documentation/swift_packages/bundling_resources_with_a_swift_package
Screen Shot 2021-10-28 at 21 58 02

https://github.com/apple/swift-package-manager/blob/77f7ac7dc49c921a5241f5e79b4bd6fa52795b25/Sources/Build/BuildPlan.swift#L661-L707

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 28, 2021

I think that's why people are not using the content of "Everything.md" to init the everythindDocument properly since the original swift version was 4.2

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Oct 28, 2021

It seems we have 3 options to go, which one do you prefer? (Personally, I prefer option2, but option3 is also acceptable)

  1. Delete the md file. use hard-coded text
  2. Use md resource file. Update the Package.swift to 5.3 (Release on Sep 17, 2020)
  3. Use md resource file. Add a fallback to provide Bundle.module before swift 5.3

@Kyle-Ye Kyle-Ye force-pushed the feature/dependence_version branch from 2853d7d to 39516c1 Compare October 30, 2021 02:57
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Nov 2, 2021

cc @franklinsch

@franklinsch
Copy link
Contributor

Thanks for investigating these options! I think updating to 5.3 is reasonable (option 2) since it was released more than a year ago.

Update to Swift 5.3 to support Bundle.module
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Nov 2, 2021

Thanks for investigating these options! I think updating to 5.3 is reasonable (option 2) since it was released more than a year ago.

Done. Could you please help me run test on this?

@franklinsch
Copy link
Contributor

@swift-ci please test this

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Nov 2, 2021

@swift-ci please test this

Looks like it does not trigger the Test check. Maybe it should be "@swift-ci please test"?

@franklinsch
Copy link
Contributor

@swift-ci please test

@franklinsch
Copy link
Contributor

@swift-ci please test this

Looks like it does not trigger the Test check. Maybe it should be "@swift-ci please test"?

Yes, just noticed this as well 😄

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Nov 2, 2021

Any other suggestion on this? Or could we merge it with your approval?

@franklinsch
Copy link
Contributor

Looking at the CI logs, it looks like it's now emitting this warning:

FormatCommand.swift:133:19: note: use 'run' instead

Can we resolve this before merging this PR?

@franklinsch
Copy link
Contributor

Actually, I see that warning happened before the changes of this PR, so we can resolve this as part of another PR.

Copy link
Contributor

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

Thanks @Kyle-Ye for another awesome PR! 🙌

@franklinsch franklinsch merged commit 6d11986 into swiftlang:main Nov 2, 2021
@Kyle-Ye Kyle-Ye deleted the feature/dependence_version branch November 2, 2021 11:36
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Nov 2, 2021

Looking at the CI logs, it looks like it's now emitting this warning:

FormatCommand.swift:133:19: note: use 'run' instead

Can we resolve this before merging this PR?

It seems to be fixed by this PR.(Which is blocked by this PR before) #3

franklinsch added a commit that referenced this pull request Nov 2, 2021
franklinsch added a commit that referenced this pull request Nov 2, 2021
Kyle-Ye added a commit to Kyle-Ye/swift-markdown that referenced this pull request Nov 3, 2021
QuietMisdreavus pushed a commit that referenced this pull request Nov 10, 2021
@Kyle-Ye Kyle-Ye mentioned this pull request Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants