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 Salsa master branch as subtree under hipcheck-incremental #404

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

Conversation

j-lanson
Copy link
Collaborator

@j-lanson j-lanson commented Sep 12, 2024

Open to discussion about whether this is the right way to include Salsa's source code in our repo.

I'm not sure what it will look like when we try to pull down new changes from Salsa's main branch. This one adds the entire codebase as a single commit, presumably if/when we update to a newer Salsa commit, we'd want 1 commit that plays all the changes between the current version and the new version.

@j-lanson j-lanson self-assigned this Sep 12, 2024
@alilleybrinker
Copy link
Collaborator

Taking a look!

@alilleybrinker
Copy link
Collaborator

Okay, some quick reviewing and I have a few thoughts:

  • It looks like the merge commit for the subtree integration doesn't meet the Conventional Commits standard. That should be easy enough to fix.
  • The Salsa crate we're importing is itself a workspace (it has a "main" top-level crate Cargo.toml that is also a workspace-root manifest, with two more crates under the components/ directory. In general, workspace-in-workspace doesn't work well, and in our case it looks like cargo-dist is upset about it.
  • We'll likely need to define some tooling for managing the subtree. I do think a subtree is probably the right thing, but I also want to investigate what Rust Analyzer does, because I know they also vendor Salsa in.

@alilleybrinker
Copy link
Collaborator

It looks like Rust Analyzer just has a copy that they manage completely by hand and rarely sync from upstream. They also make their own edits to it. So I don't think they're good prior art for us, where we likely don't want to make edits from upstream.

@alilleybrinker alilleybrinker changed the title add Salsa master has git subtree under hipcheck-incremental Add Salsa master branch as subtree under hipcheck-incremental Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

2 participants