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

Fix JSON package dump compatibility… #216

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

Conversation

kielgillard
Copy link
Contributor

@kielgillard kielgillard commented Jul 25, 2021

…since JSON package descriptions do not always seem to include target dependencies.

I'm not thrilled with this since the package dump doesn't include sources, but the path to them. Not thrilled with how complex the decoding logic is becoming.

Filed SR-14972.

Copy link
Contributor

@andrewchang-bird andrewchang-bird 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 the PR! The difference you’re seeing between package describe and dump-package is the latter is a subset that only includes dependencies defined as targets in the project manifest. There isn’t a straightforward way I can see to resolve external dependency source paths from dump-package, so the dependency graph should end up the same as package describe.

The main reason we’d choose to use dump-package is that it supports mixed (Obj-C) source projects, whereas package describe doesn’t. Supporting another schema is okay, but a few comments:

  1. When sources isn’t specified, it’s implicitly set based on path
  2. Each item in sources can be a glob, so it needs to be expanded. I think Path.glob should work for this.
  3. As you noted, the decoding logic is getting pretty complex. Let’s split the decoder into one for dump-package and one for package describe.

// https://github.com/apple/swift-package-manager/blob/main/Sources/PackageDescription/Target.swift
let name = try container.decode(String.self, forKey: .name)
let type = try container.decode(String.self, forKey: .type)
if type == "regular" || type == "library" || type == "executable" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should make this into an enum and decode optionally.

@kielgillard
Copy link
Contributor Author

kielgillard commented Jul 26, 2021

@andrewchang-bird I'm looking at the SwiftPM library and wondering if Mockingbird should use it instead of recreating all this JSON parsing, assumptions about implicit paths etc?

Going off this example, it looks like it should be possible to ask libSwiftPM to load a package and return the targets, their dependencies and target source files. It seems to offer the appropriate level of abstraction over targets, dependencies and sources.

However, at this point it this seems like the changes are evolving the JSON project feature beyond its original use case since it is basically allowing developers to pass a package as the project: mockingbird generate --project path/to/Package.swift --target Feature.

In absence of such a feature, my team and I would have to create and maintain Xcode workspaces and projects for our packages just to generate mocks. This is obviously undesirable. It seems to me this pattern of small, focused modules is gaining traction in the community, too. What do you think?

@andrewchang-bird
Copy link
Contributor

I think it’s definitely worth looking into. I’m prioritizing some big changes to the framework and generator for 0.18, so if you have the bandwidth it would help tremendously. Note that you’ll have to bump the existing version of SPM that’s being linked in for argument parsing. Not sure about the compatibility, but the CLI should probably use the newer Swift Argument Parser lib instead regardless.

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