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

Replace git submodule with package dependency for tree-sitter integration #23

Merged
merged 3 commits into from
Apr 13, 2024
Merged

Replace git submodule with package dependency for tree-sitter integration #23

merged 3 commits into from
Apr 13, 2024

Conversation

yeatse
Copy link
Contributor

@yeatse yeatse commented Apr 13, 2024

Some of the dependencies of my project (like Runestone) rely on the tree-sitter project directly, which leads to conflicts due to duplicated symbols. This PR tries to fix this by replacing the tree-sitter git submodule with a Swift package dependency.

Note that the tree-sitter-swift submodule remains unchanged in this PR because it is currently used only for testing purposes, and altering it would introduce unnecessary dependencies.

@mattmassicotte
Copy link
Contributor

Thank you so much for this! I've been thinking about this change for a very long time myself.

It's a shame this conflict is even a thing. I'd love to find a way to share more significant infrastructure with Runestone aside from just the runtime. But, until that day comes, I think this is a good tradeoff for the sake of compatibility.

@mattmassicotte
Copy link
Contributor

Ah looks like perhaps some tests still need to be updated?

@yeatse
Copy link
Contributor Author

yeatse commented Apr 13, 2024

Ah looks like perhaps some tests still need to be updated?

Fixed

@yeatse
Copy link
Contributor Author

yeatse commented Apr 13, 2024

Some of the tests are still failing. Let me have a look.

@mattmassicotte
Copy link
Contributor

The tree-sitter package has a higher-than-required watchOS version. Lowering that will be a pain, so let's just raise it here.

@yeatse
Copy link
Contributor Author

yeatse commented Apr 13, 2024

The tree-sitter package has a higher-than-required watchOS version. Lowering that will be a pain, so let's just raise it here.

From the documentation if we don't specify a platform requirement, the oldest deployment targets will be selected by default. So we can just remove it to keep consistency with the tree-sitter package.

@mattmassicotte
Copy link
Contributor

Ah. The documentation is right! Leaving it out is fine if you don't actually rely on the implicit value. If it does matter, clients that change to a lower swift version will implicitly lower yours and cause breakage.

In general, I've found it to be a rare, but annoying compatibility issue when left blank.

@mattmassicotte
Copy link
Contributor

But, I also don't want to make too much work for you. I'lll deal with that if it comes up!

@mattmassicotte mattmassicotte merged commit b39502d into ChimeHQ:main Apr 13, 2024
6 checks passed
@yeatse yeatse deleted the package-dependency branch April 13, 2024 13:44
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