Skip to content

feat: add migrate-to-spm cli command #7963

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

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

feat: add migrate-to-spm cli command #7963

wants to merge 28 commits into from

Conversation

markemer
Copy link
Member

@markemer markemer commented Apr 7, 2025

No description provided.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Added some comments.

Also:

  • ios-spm-template already has the same files you created in the ios-spm-migrate-template, can't you use ios-spm-template files instead of a new ones?

  • Do we need the --unsafe and --dry-run options? for --unsafe I assume people uses git or some other source control, so they could revert if they had some problem. And for --dry-run we are documenting the changes the command does so people can know.

  • We have a separate ticket for the messaging/handling of non SPM plugins, so you can leave those changes out of the migration command.

@jcesarmobile
Copy link
Member

Another thought, since it doesn't do a full migration, maybe it should be renamed to assist-spm-migration?

And at the end it should show a link to the SPM docs, to the ### Using our migration tool section once it's live.

@markemer
Copy link
Member Author

Added some comments.

Also:

  • ios-spm-template already has the same files you created in the ios-spm-migrate-template, can't you use ios-spm-template files instead of a new ones?
  • Do we need the --unsafe and --dry-run options? for --unsafe I assume people uses git or some other source control, so they could revert if they had some problem. And for --dry-run we are documenting the changes the command does so people can know.
  • We have a separate ticket for the messaging/handling of non SPM plugins, so you can leave those changes out of the migration command.
  • I can look into the best way to use the template files - it might be trivial to do that.
  • I'm gonna ditch --unsafe but keep --dry-run, I think
  • Non SPM plugins will just throw a warning for now - but I don't want to give people the idea it worked when it didn't.

@markemer
Copy link
Member Author

Added some comments.
Also:

  • ios-spm-template already has the same files you created in the ios-spm-migrate-template, can't you use ios-spm-template files instead of a new ones?
  • Do we need the --unsafe and --dry-run options? for --unsafe I assume people uses git or some other source control, so they could revert if they had some problem. And for --dry-run we are documenting the changes the command does so people can know.
  • We have a separate ticket for the messaging/handling of non SPM plugins, so you can leave those changes out of the migration command.
  • I can look into the best way to use the template files - it might be trivial to do that.
  • I'm gonna ditch --unsafe but keep --dry-run, I think
  • Non SPM plugins will just throw a warning for now - but I don't want to give people the idea it worked when it didn't.

So the npm tar package seems not to respect the file list you give it. I even enumerated every file, so I think the separate tarball works better - although we could at some point generate it directly from the template directory.

@markemer markemer requested a review from jcesarmobile April 15, 2025 19:25
@jcesarmobile
Copy link
Member

So the npm tar package seems not to respect the file list you give it. I even enumerated every file, so I think the separate tarball works better - although we could at some point generate it directly from the template directory.

I don't understand what you mean in the comment, but can't you extract it to a temp folder and copy the needed files to the user project?
I really don't want a separate template with duplicate files we have to maintain

@markemer
Copy link
Member Author

So the npm tar package seems not to respect the file list you give it. I even enumerated every file, so I think the separate tarball works better - although we could at some point generate it directly from the template directory.

I don't understand what you mean in the comment, but can't you extract it to a temp folder and copy the needed files to the user project? I really don't want a separate template with duplicate files we have to maintain

Yeah, /tmp might be the way - tar should allow me to just untar the stuff I want - extract accepts a file list, but it seems to totally ignore it.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

There are several comments that have not been addressed yet

@markemer markemer requested a review from jcesarmobile April 21, 2025 17:36
@markemer
Copy link
Member Author

There are several comments that have not been addressed yet

I just pushed the removal of Cap-SPM as a separate zip file, should be ready now.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

The changes from https://github.com/ionic-team/capacitor/pull/7982/files need to be applied on the migration

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

The debug.xcconfig is not being added to the project (https://github.com/ionic-team/capacitor/blob/main/ios-spm-template/debug.xcconfig) and I think it should, or at least should be copied to the project folder and tell the users to manually add it to the Xcode project or explain how to create a new one, both in here and in the docs PR. But adding it or creating it and also adding the CapApp-SPM dependency seem like too much manual changes for a "migrate" command, so would be cool if the command could add it.

The Package.swift detection is not working properly, I get

Found 6 Plugins with Package.swift files:
@capacitor-community/[email protected]

and then

[warn] CapacitorCommunityTapJacking does not have a Package.swift

The plugin doesn't have a Package.swift, so the warning is correct, but shouldn't be listed as having it.
Anyway, I told you to remove that part from this PR since we have another ticket for that task and there are some other things that should be changed related to it, so better focus just on the migration task.

Also, can you remove the comments about you hating things?

@markemer
Copy link
Member Author

markemer commented May 7, 2025

The Package.swift detection is not working properly, I get

Found 6 Plugins with Package.swift files:
@capacitor-community/[email protected]

and then

[warn] CapacitorCommunityTapJacking does not have a Package.swift

Yeah, that's a new one on me - I'll take a look at why that's failing. The rest are no problem.

Also, can you remove the comments about you hating things?

Oh yeah, I didn't even mean to check that in. Moment of frustration with TS and all that.

Since I'm in here, I'll also change the name - it's gotten far enough away that I think you're right and I'm gonna call it spm-migration-assistant

@markemer markemer requested a review from jcesarmobile May 7, 2025 20:07
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.

3 participants