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

Update Yams dependency to "from: 4.0.0" #379

Merged
merged 1 commit into from
Mar 9, 2022
Merged

Conversation

e28eta
Copy link
Contributor

@e28eta e28eta commented Mar 8, 2022

Manually building toolbox-18.3.5 on macOS logs the following compiler warnings:

Working copy of https://github.com/jpsim/Yams.git resolved at 2.0.0
/Users/daniel/Downloads/toolbox-18.3.5/.build/checkouts/Yams/Sources/Yams/Emitter.swift:338:32: warning: initialization of 'UnsafeMutablePointer<yaml_version_directive_t>' (aka 'UnsafeMutablePointer<yaml_version_directive_s>') results in a dangling pointer
            versionDirective = UnsafeMutablePointer(&versionDirectiveValue)
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/daniel/Downloads/toolbox-18.3.5/.build/checkouts/Yams/Sources/Yams/Emitter.swift:338:53: note: implicit argument conversion from 'yaml_version_directive_t' (aka 'yaml_version_directive_s') to 'UnsafeMutablePointer<yaml_version_directive_t>' (aka 'UnsafeMutablePointer<yaml_version_directive_s>') produces a pointer valid only for the duration of the call to 'init(_:)'
            versionDirective = UnsafeMutablePointer(&versionDirectiveValue)
                                                    ^~~~~~~~~~~~~~~~~~~~~~
/Users/daniel/Downloads/toolbox-18.3.5/.build/checkouts/Yams/Sources/Yams/Emitter.swift:338:53: note: use 'withUnsafeMutablePointer' in order to explicitly convert argument to pointer valid for a defined scope
            versionDirective = UnsafeMutablePointer(&versionDirectiveValue)

This issue was solved in Yams v3.0, almost 2 years ago. Looking through the version history, there are other bug fixes, which may/may not have any impact on vapor.

Given that vapor's documentation says Swift 5.2+ is required, and Yams v4 only requires 5.1+ (but Yams v5 requires 5.4+), this feels like a good dependency update to make.

I wasn't able to do a great job testing this, but I think it's working correctly.

  • It compiles and runs for me.
  • vapor new works with the default template.
  • I only found one other template that uses a manifest.yml when searching for vapor + template topics (per recommendation in older versions of the docs). That template also works.

This seems to be the only usage of Yams in the toolbox, that I could find.

let gitUrl = signature.templateURL ?? "https://github.com/vapor/template"
let cwd = FileManager.default.currentDirectoryPath
let workTree = signature.outputDirectory?.asDirectoryURL.path ?? cwd.appendingPathComponents(name)
let templateTree = workTree.deletingLastPathComponents().appendingPathComponents(".vapor-template")
context.console.info("Cloning template...")
try? FileManager.default.removeItem(atPath: templateTree)
let gitBranch = signature.templateBranch ?? "main"
_ = try Process.git.clone(repo: gitUrl, toFolder: templateTree, branch: gitBranch)
if FileManager.default.fileExists(atPath: templateTree.appendingPathComponents("manifest.yml")) {
try FileManager.default.createDirectory(atPath: workTree, withIntermediateDirectories: false, attributes: nil)
let yaml = try String(contentsOf: templateTree.appendingPathComponents("manifest.yml").asFileURL, encoding: .utf8)
let manifest = try YAMLDecoder().decode(TemplateManifest.self, from: yaml)

Vapor 4 docs say Swift 5.2 is required, this brings Yams up to minimum Swift 5.1 and includes bug fixes.
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@0xTim 0xTim merged commit 951cfe0 into vapor:main Mar 9, 2022
@e28eta e28eta deleted the patch-1 branch March 10, 2022 08:39
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