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

Closed
wants to merge 3 commits into 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
@alilleybrinker alilleybrinker force-pushed the jlanson/add-hipcheck-incremental branch from 17d4dad to 10d92b6 Compare September 13, 2024 18:31
@alilleybrinker
Copy link
Collaborator

@j-lanson I think it makes sense to close this. Once RFD 7 is approved, we'll plan to not publish to Crates.io for the 3.8.0 release, which will eliminate the need to vendor Salsa.

@j-lanson
Copy link
Collaborator Author

Closing this in accordance with @alilleybrinker 's comment above.

@j-lanson j-lanson closed this Sep 23, 2024
@alilleybrinker alilleybrinker deleted the jlanson/add-hipcheck-incremental branch September 24, 2024 16:31
@alilleybrinker alilleybrinker restored the jlanson/add-hipcheck-incremental branch September 24, 2024 16:31
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.

2 participants