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

@robertbastian robertbastian force-pushed the sub branch 2 times, most recently from 0d7893c to ec43edf Compare October 21, 2023 14:06
@robertbastian robertbastian marked this pull request as ready for review October 24, 2023 13:22
@robertbastian robertbastian requested review from Manishearth, sffc and a team as code owners October 24, 2023 13:22
@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
@robertbastian robertbastian deleted the sub branch October 26, 2023 17:48
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