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

Add ark-cli crate #13

Merged
merged 26 commits into from
Apr 8, 2024
Merged

Conversation

tareknaser
Copy link
Collaborator

This pull request adds the ark-cli crate into the ark-rust workspace.

Changes made:

Relocated all ark-cli code into a new crate.

Note: A git merge conflict occurred with the README file. I had to add a separate commit to add it.

Next steps

currently, ark-cli still depends on ark-lib

arklib = { git = "https://github.com/ARK-Builders/arklib", rev = "2c7ceda" }

We should make it depend on other crates from the workspace

@kirillt
Copy link
Member

kirillt commented Mar 12, 2024

@tareknaser tareknaser force-pushed the ark-cli-git-preserve branch from 657d48e to 28806f0 Compare March 14, 2024 03:44
The branch `ark-rust_hot_fix` contains a hot fix where we use the same commit specified when writing `ark-cli` but with pinning `image` crate to the correct version so that `ark-rust` can compile

This is just a hot fix. `arklib` shouldn't be even a dependency of any `ark-rust` crate. See ARK-Builders#15 for more information

Signed-off-by: Tarek <[email protected]>
@tareknaser tareknaser force-pushed the ark-cli-git-preserve branch from 14975d6 to 231a6ce Compare April 7, 2024 19:52
@tareknaser
Copy link
Collaborator Author

Ok so I refactored the PR

Changes Made:

  • Used git filter-repo to rewrite each commit of ark-cli under the ark-cli/ directory (documentation reference).
  • Merged the new git history of ark-cli into ark-rust (allowing unrelated histories).

Advantages:

  • Preserved the original git history of ark-cli. Authors and commit dates are correct
  • Examining commits from ark-cli doesn't show "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository" message

Disadvantages:

  • All ark-cli commits are rewritten so they are basically not the same. They don't have the same commit hashes

@tareknaser
Copy link
Collaborator Author

To justify 231a6ce:

Currently, ark-cli depends on arklib at a specific commit "2c7ceda" https://github.com/ARK-Builders/ark-cli/blob/main/Cargo.toml#L11. This commit of arklib relies on "image v0.24.2". Due to a compilation issue, we made changes in ARK-Builders/arklib#87 , shifting to "image v0.25". This change was included in ark-rust as well.

ark-rust now depends on "image v0.25". The current setup poses a compatibility issue because ark-cli relies on arklib, not other crates in ark-rust. We are planning to address this issue in #15

As a hot fix, I made a new branch https://github.com/ARK-Builders/arklib/tree/ark-rust_hot_fix aligning with the same commit as ark-cli, with an additional commit to resolve the compilation issue.

@kirillt
Copy link
Member

kirillt commented Apr 8, 2024

@tareknaser good work, I think solution using git filter-repo is OK. Commit hashes don't have to be exactly the same as in the older repo. The most important is preserving authorship and dates.

@tareknaser tareknaser merged commit 5cf25df into ARK-Builders:main Apr 8, 2024
1 check failed
@tareknaser
Copy link
Collaborator Author

Glad to see this pull request merged!

@tareknaser tareknaser deleted the ark-cli-git-preserve branch April 8, 2024 10:29
@tareknaser tareknaser restored the ark-cli-git-preserve branch April 8, 2024 10:33
@tareknaser
Copy link
Collaborator Author

I've merged the PR. The commits in ark-cli retained their original authors and no longer display the "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository" message as expected.
But it seems that the commit dates in ark-cli have been reset to today's date.

After second thought, it makes sense because ark-cli and ark-rust have unrelated git histories. To maintain the git histories of both repositories, we would need to rewrite the git history of ark-rust from scratch as well.

@kirillt What's your take on the current approach? Are you content with it, or do you lean towards rewriting the git history of ark-rust?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants