-
Notifications
You must be signed in to change notification settings - Fork 5.4k
feat: implement forc add
and forc remove
to add/remove dependencies
#7143
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
base: master
Are you sure you want to change the base?
Conversation
@IGI-111 this is ready for review. I also noticed something, i was unable to trigger the lockfile update by just updating the members_manifest, it only worked when the forc.toml was already updated with new dependencies. |
CodSpeed Performance ReportMerging #7143 will not alter performanceComparing Summary
|
You may also want to integrate with the |
This looks nice but you need to add some tests and fix the CI errors before we can consider merging it. |
Alright will do that.
…On Mon, May 5, 2025, 10:47 AM IGI-111 ***@***.***> wrote:
*IGI-111* left a comment (FuelLabs/sway#7143)
<#7143 (comment)>
This looks nice but you need to add some tests and fix the CI errors
before we can consider merging it.
—
Reply to this email directly, view it on GitHub
<#7143 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANGCG5W7LKLXHHZEX2C7FW3244XRTAVCNFSM6AAAAAB4LUHOYKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNJQGQ3TCOBXHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…JoE11-y/sway into feat/forc-edit-subcommands-impl
Hey @JoE11-y thanks for the PR. Just letting you know I plan to look at this properly on Monday. |
@kayagokalp I removed the default semver fallback. And just added the an error requesting user to specify version either via git or path or with version that way code still works in production pending when the registry lookup feature is up and running. I also updated the tests to test for that too. |
forc
*forc add
and forc remove
to add/remove dependencies
member_manifests.insert( | ||
updated_package_manifest.project_name().to_string(), | ||
updated_package_manifest, | ||
); | ||
|
||
let new_plan = pkg::BuildPlan::from_lock_and_manifests( | ||
&lock_path, | ||
&member_manifests, | ||
false, | ||
opts.offline, | ||
&opts.ipfs_node.clone().unwrap_or_default(), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seems a little off to me, the dependencies are not members of build plan Members' refer to the workspace members (packages in a workspace, indivual packages might have dependenices attached to them which this PR aims to add/remove with a command. So we are not modifying the members, but the deps of the members). I will test in a bit but my guess is that this won't even work, until then i am blocking this PR so that it doesn't get merged accidentally. I'll update in a bit
Also there is probably no need to be this verbose and do these steps manually, we should be able to run build
after modifying the manifest file, build
will run build plan generation, lock updates, and all the extra logic you have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, thank you for catching that and for all the thorough reviews so far.
I really appreciate it. I wanted to share a bit of the journey behind these changes:
-
I initially pulled the target package directly from the workspace’s
member_manifests()
and updated it in place, expecting that to regenerate the build plan and lockfile. -
Then I discovered that updating only the in-memory manifest wasn’t enough to trigger the lockfile update. my first comment to the pr
-
So with the help of and with subsequent revisions from @zees-dev , I was able to rework the pr, to focus on updating the toml file directly, performing the checks along the way.
I’ve spent countless hours on this iteration, tweaking, testing, and making sure it behaves exactly like its sibling commands. I’m confident that this approach is rock solid.
And yes that logic was from the old design.
simply shadowing it with the updatedPackageManifest will clear it up
let member_manifests = updated_package_manifest.member_manifests()?;
Thanks again for your patience and feedback. I’m excited to get this merged! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey again, what i meant was that you shouldn't need to modify member_manifest
at all because:
- What you are implementing here is not related to member manifests, added dependency should not be part of member manifest.
- It is not needed to do this manually, build plan generation handles all the steps you are doing manually, you just need to update the manifest file (forc.toml) and regenerate the build plan that will take care of everything :)
I would simply write the updated forc.toml to the disk and call build/check, or if you want to BuildPlan::from_pkg_options
, this way you don't have to manage anything yourself.
If this doesn't help, give me some time, I'll prepare you a moredetaild write up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh for
- that was the old design.
It's only being used to properly verify the package path.
As we know from cargo.
We can call the add command from either the root of a workspace or from the root of a package.
So if called from root of a workspace the package where the dependency is to be added/removed should be specified with the --package flag.
So checks to ensure that specified package is included in member_manifests are required.
Then also when you want to add another member of a workspace as a dependency to another member.
Those scenarios are where we need to access the member_manifests
That insert was part of the old design where I figured I could take out the exact manifest and then update.
- Yeah that could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested the version at 6a78549 seems to be working, i am fine to keep it working and get it merged we can clean it later on :) Let's revert back to it, and I will do a refactor over your code after merging it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this! Thanks :) Approving this as this is getting unnecessarily long PR history, and the feature seems to be working.
My final comments are:
- The logic to add/remove packages a little too convoluted and there are areas of improvements to seriously cut down the code size.
forc add
without any dep to add doesn't produce any error/warning simply does nothing silently which is not great- The way the feature is implemented will be removing all the comments present in the Forc.toml
But given that you already put so much work on this, I think we can merge this as it is, and later on I can do a small refactor to keep this code section little bit smaller by using the existing code-paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving again; thanks for your contribution @JoE11-y!
Any additional work required should probably be captured in a TODO and/or GH issue - so it can be addressed at some point.
I think we can handle the forc add/remove failure by adding the required field in clap for them.
Thank you @zees-dev and @kayagokalp ❤️ |
@zees-dev @kayagokalp resolved the comment removal issue and the forc add without dep failure. Thanks for helping reach this stage of the pr. And my apologies for being a little over zealous.🫡 |
Thanks for the contribution! Before we can merge this, we need @sdankel to sign the Fuel Labs Contributor License Agreement. |
Description
Close #2369
Summary:
Implements functionality for
forc
to add remove dependencies through cli, similar to Cargo'sadd
andremove
commands.Work Done:
forc add
command to support adding both regular and contract dependencies from path, git, IPFS, or version.forc remove
to cleanly remove specified dependencies from the manifest.DepSection
enum for unified regular/contract dependency handling.toml_edit
to update the manifest file while preserving formatting.Checklist
Breaking*
orNew Feature
labels where relevant.