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

[docs] Added documentation for dependency overrides #11253

Merged
merged 3 commits into from
Apr 25, 2023
Merged

Conversation

awelc
Copy link
Contributor

@awelc awelc commented Apr 24, 2023

Description

A description of how recently landed (#11181) dependency overrides work.

Please let me know if you need this changes, extended, etc.

I am also not sure if this should be a separate file (as it is intended for now) or if it should be folded into an existing document (e.g., https://github.com/MystenLabs/sui/blob/main/doc/src/build/package-upgrades.md), and if the former is the case, if there is any additional work required to have it properly displayed on the developer portal.

Test Plan

It's just the docs


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

@awelc awelc requested a review from amnn April 24, 2023 14:31
@awelc awelc self-assigned this Apr 24, 2023
@vercel
Copy link

vercel bot commented Apr 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Apr 24, 2023 4:11pm
explorer-storybook ⬜️ Ignored (Inspect) Apr 24, 2023 4:11pm
sui-wallet-kit ⬜️ Ignored (Inspect) Apr 24, 2023 4:11pm
wallet-adapter ⬜️ Ignored (Inspect) Apr 24, 2023 4:11pm

@github-actions github-actions bot added the Type: Documentation Improvements or additions to documentation label Apr 24, 2023
Copy link
Contributor

@ronny-mysten ronny-mysten left a comment

Choose a reason for hiding this comment

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

Thanks @awelc ! I've added a Jira for myself to add this topic to the navigation, too.

doc/src/build/dependency-overrides.md Outdated Show resolved Hide resolved
doc/src/build/dependency-overrides.md Outdated Show resolved Hide resolved
doc/src/build/dependency-overrides.md Outdated Show resolved Hide resolved
doc/src/build/dependency-overrides.md Outdated Show resolved Hide resolved
doc/src/build/dependency-overrides.md Outdated Show resolved Hide resolved
Co-authored-by: ronny-mysten <118224482+ronny-mysten@users.noreply.github.com>
@awelc
Copy link
Contributor Author

awelc commented Apr 24, 2023

Thanks @awelc ! I've added a Jira for myself to add this topic to the navigation, too.

I have accepted all the suggested changes but will wait for @amnn's feedback on what may have to be changed or added.

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Thanks @awelc ! Content looks good, modulo some small comments.

doc/src/build/dependency-overrides.md Outdated Show resolved Hide resolved
crypto = { git = "https://github.com/crypto.org/crypto.git" , rev = "v2.0.0"}
```

This situation represents the [_diamond dependency conflict_](https://jlbp.dev/what-is-a-diamond-dependency-conflict) where two indirect dependencies in turn depend on different versions of the same package. If all packages involved are developed independently, then the developer of the `user` package has a problem -- the Move compiler cannot discern which version of the `crypto` package to use and, as a result, building of the `user` package fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

https://jlbp.dev/what-is-a-diamond-dependency-conflict

I think I picked this link originally when I was writing our internal design document, but this may not be suitable for our public-facing docs. Thoughts @ronny-mysten / @randall-Mysten ? I think it's fine to drop the link entirely, to be honest, because we're explaining what it is below.

crypto = { git = "https://github.com/crypto.org/crypto.git" , rev = "v2.0.0" , override = true }
```

Note that overriding transitive dependencies in such a manner might not always be successful. In particular, the `vault` package in our example might truly depend on the functionality of the `crypto` package that is only available in version `1.0.0` of this package (e.g, was removed in later package revisions), in which case the build would also be unsuccessful. By offering the ability to enforce dependency overrides we merely put another tool into the developer's toolbox in the hope that it would allow resolution of transitive dependency conflicts in the majority of cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't have to be in this PR, but we should also explain the on-chain constraint around versioning and compatibility, and relatedly, the lack of a relationship between on-chain and local versions. I.e. in practice, if folks are using semantic versioning, then v2.0.0 is not going to be compatible with v1.0.0 and so it won't be a package upgrade in the technical sense, and you won't be able to perform the override, and they may choose an override that makes everything compile, but again find themelves in hot water because it's not a strictly greater version on-chain.

This is another place where Automatic Address Management (or an extension of it) will help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! And I tried to think what the best place for the additional information would be and I am currently leaning towards a separate "page", perhaps titled "Package Versioning" (or "Code Versioning" to avoid title overlap with "Package Upgrades"). I will start writing it soon-ish (unless someone preempts me, but then please let me know :-) ), but will likely as @tzakian to fill in the compatibility part. If we decide to put it with "Dependency Overrides" after all, we can indeed always augment this document in a separate PR.

Co-authored-by: Ashok Menon <ashok@mystenlabs.com>
@awelc awelc merged commit 92b7739 into main Apr 25, 2023
@awelc awelc deleted the aw/dep-override-doc branch April 25, 2023 07:42
ronny-mysten added a commit that referenced this pull request May 2, 2023
## Description 

As a follow-up to #11253, added
some additional info on package upgrades/versioning that (I believe) is
not yet covered anywhere.

I feel like perhaps we should pull some introductory material on upgrade
policies from
https://github.com/MystenLabs/sui/blob/main/doc/src/build/custom-upgrade-policy.md
(@amnn?) but I did not want to make changes that are too disruptive at
this point.

@tzakian - please chime in if the description of the compatibility
checks, admittedly pretty short, is sufficient or if we should
add/change something right away.

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [x] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes

---------

Co-authored-by: Ashok Menon <ashok@mystenlabs.com>
Co-authored-by: Ronny Roland <ronmilton@mystenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants