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

Move diplomacy to standalone library absorbing aop #3571

Merged

Conversation

lordspacehog
Copy link
Contributor

Related issue: #3037

Type of change: other enhancement

Impact: API Deprecation in favor of standalone diplomacy

Development Phase: implementation

Release Notes
Splits out the aop and diplomacy modules into a standalone library. Adds the new library as a dependency and updates the in-tree diplomacy and aop modules to reference the new library with deprecation warnings.

@lordspacehog lordspacehog marked this pull request as draft February 19, 2024 18:27
@lordspacehog lordspacehog force-pushed the aswehla/standalone_diplomacy branch from bf556df to f21658b Compare February 21, 2024 20:50
@jerryz123
Copy link
Contributor

Very nice, I greatly appreciate the effort to preserve the deprecated package paths.

@lordspacehog lordspacehog marked this pull request as ready for review February 23, 2024 00:47
@lordspacehog lordspacehog changed the title [WIP] Move diplomacy to standalone library absorbing aop Move diplomacy to standalone library absorbing aop Feb 23, 2024
@lordspacehog
Copy link
Contributor Author

yeah, I mostly followed the lead of @sequencer and attempted to capture as minimal of a change as possible for this initial split. I also have other PRs ready to go that update each of the individual modules internally to use the new diplomacy library directly.

I've also run tests locally and everything seems to be passing without issue. so this should be ready for review.

Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LTGM, really really really thank to your rebasing!!!
There are some nitpits in the commit. please keep the patch as minimal as possible. I'll take a look at your reabsed version of diplomacy today!

.scalafmt.conf Outdated Show resolved Hide resolved
.mill-version Outdated Show resolved Hide resolved
.gitmodules Outdated
@@ -7,3 +7,6 @@
[submodule "dependencies/chisel"]
path = dependencies/chisel
url = https://github.com/chipsalliance/chisel.git
[submodule "dependencies/diplomacy"]
path = dependencies/diplomacy
url = https://github.com/lordspacehog/diplomacy.git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be chipsalliance/diplomacy in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, My current plan is to do a 2nd PR to cut it over,. or we can wait until diplomacy is published to ivy and i can just the setup over to use ivy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated this PR to point at the new branch in the chipsalliance/diplomacy repo

build.sc Outdated Show resolved Hide resolved
build.sc Outdated Show resolved Hide resolved
build.sc Outdated Show resolved Hide resolved
build.sc Outdated Show resolved Hide resolved
build.sc Outdated Show resolved Hide resolved
common.sc Outdated Show resolved Hide resolved
src/main/scala/diplomacy/Resources.scala Outdated Show resolved Hide resolved
@sequencer
Copy link
Member

@lordspacehog Can you give me an small memo on how to split the diplomacy, I used to have my own way, and I need to verify your version to carefully review it:)

@lordspacehog
Copy link
Contributor Author

@sequencer This new diplomacy split is a new history pull from rocket-chip/dev using git filter-repo (https://github.com/newren/git-filter-repo). You should either force push over your local master or use a different branch name pointed to the new history for now.

The exact process i used was as follows:

  1. Fork sequencer/diplomacy
  2. clean checkout rocket-chip/dev in new folder.
  3. use git filter-repo to prune all history unrelated to the changes to the diplomacy module.
  4. add my diplomacy fork as upstream and push new history to main branch
  5. move master branch to master-archive and set main as the default branch

From there i manually went through and reconciled the changes between your diplomacy repo, the main chipsalliance diplomacy repo, and the changes that happened directly in the rocket-chip repo.

@lordspacehog lordspacehog force-pushed the aswehla/standalone_diplomacy branch from f21658b to 2f2f469 Compare February 26, 2024 22:49
@sequencer
Copy link
Member

@lordspacehog I sent u an invitation, please directly push to a branch to chipsalliance/diplomacy, and I can take a look ;p

@lordspacehog
Copy link
Contributor Author

@sequencer accepted the invite, but it looks like i don't have push access to the repo, only triage. if you can update that i'll get a branch pushed ASAP. i'll also stage an update to this PR to set that new branch as the submodule target.

@sequencer
Copy link
Member

@sequencer accepted the invite, but it looks like i don't have push access to the repo, only triage. if you can update that i'll get a branch pushed ASAP. i'll also stage an update to this PR to set that new branch as the submodule target.

Access granted.

@lordspacehog
Copy link
Contributor Author

@lordspacehog lordspacehog force-pushed the aswehla/standalone_diplomacy branch 2 times, most recently from 58217be to 76fa451 Compare February 28, 2024 19:05
Splits out the aop and diplomacy modules into a standalone
library. Adds the new library as a dependency and updates the
in-tree diplomacy and aop modules to reference the new library with
deprecation warnings.
@lordspacehog lordspacehog force-pushed the aswehla/standalone_diplomacy branch from 76fa451 to 27f23ee Compare February 28, 2024 19:58
@sequencer
Copy link
Member

cc @lordspacehog, I reviewed and pushed the master branch to https://github.com/chipsalliance/diplomacy. please set that as upstream and go though CI;p

Updates the diplomacy dependency submodule to track
chipsalliance/diplomacy master branch.
@lordspacehog
Copy link
Contributor Author

@sequencer updated the submodule reference.

@sequencer sequencer merged commit b3476b1 into chipsalliance:dev Mar 8, 2024
26 checks passed
@sequencer
Copy link
Member

it might have additional things to clean up. Merge it for now. I’ll take care of them later this week

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