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

Using a git submodule for Diplomat #4180

Closed
wants to merge 1 commit into from

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Oct 19, 2023

This makes it easier to iterate on ICU4X and Diplomat in parallel. It also simplifies CI as it can use the local diplomat-tool instead of having to cargo install it.

@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Oct 19, 2023
@sffc
Copy link
Member

sffc commented Oct 19, 2023

Not opposed but we should discuss this because adding a git submodule increases the complexity for everyone hacking on ICU4X even if they don't interact with Diplomat/FFI.

@robertbastian
Copy link
Member Author

Yes we should discuss.

@robertbastian robertbastian added the discuss-priority Discuss at the next ICU4X meeting label Oct 20, 2023
@robertbastian
Copy link
Member Author

I've achieved most of what I wanted to achieve here with rust-diplomat/diplomat#348 and #4197

@sffc
Copy link
Member

sffc commented Oct 26, 2023

Discussion:

  • @sffc - The number of people this helps is smallish, and the number of people it annoys is large. If you need fast iteration on diplomat locally, you can patch it locally to use path dependencies.
  • @robertbastian - The changes I made in upstream about dependencies make the git submodule mostly obsolete, because you can now patch it locally.

Conclusion: Not worth the submodule.

LGTM: @robertbastian @Manishearth @sffc

@sffc sffc closed this Oct 26, 2023
@sffc sffc removed discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting labels Oct 26, 2023
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