Pass options to Accio as comments in the Package.swift file

[acecilia]

This is a working and tested implementation that solves https://github.com/JamitLabs/Accio/issues/54.

It is using comments in the Package.swift file to pass additional configuration to Accio. I added the following keys for now:

• product-type: default, static-framework or dynamic-framework.
• integration-type: default (binary), source (similar to https://github.com/Carthage/Carthage#using-submodules-for-dependencies) or cocoapods.

This PR includes parsing those options and adding them to the Framework object. It does not include the implementation for building static frameworks, neither the implementation of the source or cocoapods integration options.

Example:

// swift-tools-version:5.0
import PackageDescription

let package = Package(
    name: "XcodeProjectName",
    products: [],
    dependencies: [
        .package(url: "https://github.com/Flinesoft/HandySwift.git", .upToNextMajor(from: "2.8.0")),
        .package(url: "https://github.com/Flinesoft/HandyUIKit.git", .upToNextMajor(from: "1.9.0")),
        .package(url: "https://github.com/Flinesoft/Imperio.git", .upToNextMajor(from: "3.0.0")),
        .package(url: "https://github.com/JamitLabs/MungoHealer.git", .upToNextMajor(from: "0.3.0")),
        .package(url: "https://github.com/SwiftyBeaver/SwiftyBeaver.git", .upToNextMajor(from: "1.6.2")),
    ],
    targets: [
        .target(
            name: "AppTargetName",
            dependencies: [
              // accio product-type:static-framework
              "HandySwift",
              "HandyUIKit",
              // accio integration-type:cocoapods
              "Imperio",
              // accio integration-type:default
              "MungoHealer",
              "SwiftyBeaver",
            ],
            path: "TestProject-iOS"
        ),
    ]
)

For more information about how this works, have a look at the README.md file, or at the testValidComments() function in the ManifestCommentsHandlerServiceTests.swift file.

[acecilia]

@Dschee Thanks for the review. About your three points:

1. I used a syntax which is common in several swift related tools, for example: marathon, beak or SwiftLint. I do not have any preference about it, and your proposal makes sense.
2. I do not understand how you want the code to be organised: fetching a build setting from an Xcode project requires a simpler implementation. Parsing the comments on the manifest requires parsing the whole manifest, that is why ManifestCommentHandlerService exists: to hold the parsing logic and the parsing results.
3. My bad, I may not have been 100% clear: of course I was not expecting to merge this with non-implemented options. I added them just to give an idea of the possible options, and how they look in the implementation. Disabling this options just requires to remove/comment the related cases from the ProductType and IntegrationType enums, which I was planning to do before merge.

I will continue with this PR as soon as you specify in https://github.com/JamitLabs/Accio/issues/54 what is exactly what you are willing to accept/merge:

1. A comment syntax that you are happy with.
2. In case you do not like the ManifestCommentHandlerService approach, a specification of how you want to organise the code.

[mrylmz]

I would prefer @acecilia's comment style because we don't have a simple way to express an enum or a DictionaryLiteral and also keeping track of the , balancing or correctness at the end of multiple single-line comments which have no contextual awareness of the surrounding swift source would be hard to solve...

Considering that the SwiftPM contributors also make use of comment parsing like // swift-tools-version:5.0 lets me also feel :+1:

[mrylmz]

@acecilia Are you already associating the parsed RawComment(s) with the dependencies?

[acecilia]

@mrylmz Yes, this PR covers parsing of the comments and setting their content into the additionalConfiguration property inside the Frameworks object.

[Jeehut]

@acecilia I thought I already laid out the way I think "additional configuration" should be tackled in the second part above:

I don't think that we should have a ManifestCommentHandlerService or even the additionalConfiguration which are way too generic. Instead, we should handle each addition in a domain-specific way and only document the general approach in the CONTRIBUTING.md file linking to examples like this where a similar approach was already taken.

And in this comment on #54.

Just in case, here's the gist of it:

1. I don't want to extend Package.swift too much without pitching the changes directly to SwiftPM, too and try to sneak it into SwiftPM officially. This has only become so much more important now that we can actually use SwiftPM in Xcode directly and Accio has become less important for many developers.
2. If we really need so many options that it doesn't make sense to add them to Package.swift in a Swift style, then we should add a configuration file (maybe called .accio.toml) where we can add whatever we need. I'm in favor of TOML rather than YAML for this as it's much cleaner, but both are fine for me.

If you still have questions, please ask more specific questions so I understand what to focus on.

[acecilia]

1. About the comment syntax. Based on your comment in https://github.com/JamitLabs/Accio/issues/54 I implemented this PR. You did not like it, and partially proposed a different syntax. I would like a full specification of it that you are willing to approve: I think the best way you can do this is to provide an example of a Package.swift with comments indicating the integration-type and product-type for several dependencies, so I can implement it.

2. About how the code should be organised and where the additional configuration should be placed. As you say, you already provided some ideas about this:

   I don't think that we should have a ManifestCommentHandlerService or even the additionalConfiguration which are way too generic. Instead, we should handle each addition in a domain-specific way and only document the general approach in the CONTRIBUTING.md file linking to examples like this where a similar approach was already taken.

   But I commented that I do not understand them:

   I do not understand how you want the code to be organised: fetching a build setting from an Xcode project requires a simpler implementation. Parsing the comments on the manifest requires parsing the whole manifest, that is why ManifestCommentHandlerService exists: to hold the parsing logic and the parsing results.

   I will need a more detailed specification on this:

   • Instead of having a ManifestCommentHandlerService.swift, which new files should this PR contain? Where should the parsing logic be placed?
   • Where will the additionalInformation property be declared if not in the Framework object?

I do not want to again spent several hours on something that can not be approved, or you consider flawed.

[Jeehut]

1. Quote:

targets: [
    .target(
        name: "...",
        dependencies: [
            "...", "..."
        ],
        // defaultLinking: Linking.static,
        // customLinkings: [Linking.dynamic: ["HandySwift", "HandyUIKit"]],
        path: "..."
]

Something like this is what I already suggested above.

2. Like this: https://github.com/JamitLabs/Accio/blob/76646a26236f0229ec07c5b44a7af08533cc0ca9/Sources/AccioKit/Services/XcodeProjectGeneratorService.swift#L78 as I have already mentioned twice, I think.

And again, no ManifestCommendHandlerService.swift and no additionalInformation. Instead, read the necessary options only if needed. E.g. when a user uses something like the --cocoapods option. so you need to add a new option to install and update. Then, you can really solve it like in the link above. Or doesn't that work for you?

[acecilia]

1. About the comment syntax. @mrylmz already raised several concerns with that approach you proposed, which I consider valid. In addition to those, in case something like this is implemented in SPM, I see more probable that the implementation looks something like the following:

   targets: [
       .target(
           name: "...",
           dependencies: [
               "HandySwift", // Using linking by default, which is dynamic
               .dependency(name: "HandyUIKit", linking: "static")
           ],
           path: "..."
   ]

   With that syntax, what you commented is not possible:

   so sometime in the future we might simply uncomment the additional configuration code and it would just work as expected

2. About how the code should be organised and where the additional configuration should be placed.

   1. No, that does not work. The integration-type configuration is per dependency. I do not see how you envision this working when you pass the integration-type as a command line option.
   2. I can get rid of the additionalInformation property, but I will still need the ManifestCommentHandlerService file to contain the parsing logic and hold the parsing results: getting the comments for one dependency requires parsing the whole manifest, and you do not want to do that multiple times.

Also, keep in mind that, if something that you comment is not clear, quoting it is not going to make me understand it. I would suggest you try to rephrase it or explain it better.

[Jeehut]

Sorry for not mentioning this, I have already talked to @mrylmz in person and we agreed that his concerns are all not a problem. The rationale is simple: If we use a regex, the issues with the , or non-existing enum types are all not valid.

Also, the argument with the existing // swift-tools-version:5.0 is not an excuse to use this kind of comment style – the only reason they opted for this was because before they can parse the configuration, they need to know which manifest version to do this against. So they had no choice but use a different style for that. But there's no reason to use that style for anything else, so I highly doubt there ever will be other options in the manifest file with that comment syntax. That would hurt the design goals of the manifest format.

Regarding your own concern and your suggestion, the problem is that using something like .dependency(name: "HandyUIKit", linking: "static") is impossible, as this will break the code checkout process of SwiftPM and Accio will no longer work as is. We need to keep everything already defined in SwiftPM as is and – if we add configuration to the manifest file – do this by introducing new arguments or options, not by changing current behavior. If we were free to change anything though, you're right, I'd agree so use something like you suggested, but unfortunately that's not possible. That's also why I stated "we might simply uncomment the additional configuration code and it would just work". It's just a second thought that we should follow if possible – and stay as close to it as possible if it's not possible.

Regarding point 2, feel free to keep whatever implementation you like, I won't mind. What I wanted to communicate was that I don't want that comment style and therefore I don't think a "generalized" type to parse all comments in that style make any sense. But if there are comments in another style and we can write one parser for all of them to prevent multiple parsings, then I'm okay with that. I just don't think it would be such a big deal if we parsed the file multiple times given that the manifest file usually never is very long, so that might be left open for later cases, where it actually becomes a problem. But again, if you want to fix it from the start, feel free to do so.

[acecilia]

Okey. Then, the syntax is:

...
dependencies: [
    "HandySwift",
    "HandyUIKit",
    "MungoHealer",
    "SwiftyBeaver",
    "SwiftyUserDefaults",
],
// defaultLinking: .static,
// customLinking: [.dynamic: ["SwiftyBeaver", "SwiftyUserDefaults"]],
// defaultIntegration: .source,
// customIntegration: [.binary: ["HandySwift", "MungoHealer"], .cocoapods: ["HandyUIKit"]],
...

If not set explicitly using the comments, the default linking is dynamic and the default integration is binary. Can we agree on this syntax specification?

[Jeehut]

Looks good to me 👍