Skip to content
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

Bump Swift version #4

Merged
merged 5 commits into from Nov 2, 2021
Merged

Conversation

Kyle-Ye
Copy link
Collaborator

@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
Collaborator Author

Kyle-Ye commented Oct 17, 2021

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

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 27, 2021

Any idea on this PR @QuietMisdreavus ❤️

@Kyle-Ye Kyle-Ye mentioned this pull request Oct 27, 2021
1 task
Package.swift Outdated Show resolved Hide resolved
@Kyle-Ye
Copy link
Collaborator 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
@franklinsch
Copy link
Member

@swift-ci please test

@franklinsch
Copy link
Member

When I delete the Package@swift-5.5.swift 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
Copy link
Collaborator Author

Kyle-Ye commented Oct 28, 2021

When I delete the Package@swift-5.5.swift 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 Package@swift-5.5.swift 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
Collaborator 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
Collaborator 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

Fix "Package@swift-5.5.swift not pass the license check" issue
@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Nov 2, 2021

cc @franklinsch

@franklinsch
Copy link
Member

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
Collaborator 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
Member

@swift-ci please test this

@Kyle-Ye
Copy link
Collaborator 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
Member

@swift-ci please test

@franklinsch
Copy link
Member

@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
Collaborator Author

Kyle-Ye commented Nov 2, 2021

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

@franklinsch
Copy link
Member

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
Member

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

Copy link
Member

@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 apple:main Nov 2, 2021
@Kyle-Ye Kyle-Ye deleted the feature/dependence_version branch November 2, 2021 11:36
@Kyle-Ye
Copy link
Collaborator 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.

None yet

2 participants