[move-package] Remove version fields from package and dependency manifest sections #15148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
amnn
approved these changes
Dec 6, 2023
There was a problem hiding this comment.
This looks great, it's a nice clean up, thanks @kklas ! I've kicked off the CI run and tweaked the wording in the Release Notes (to make it clear that this change affects Move.toml files).
@rvantonder could you take a final look and eventually land it?
|
Hey @kklas -- haven't forgot about this, it's on my radar! A bit incidentally, I'm working on compiling against prior compiler versions (a draft PR of one approach is here, but note this is before the #15035 merge, so I will rebase at some point). I'll have a chance to look at pushing through this PR soon, just want to make sure to manage potential conflicts between this on-chain compilation work and the other (at a glance, I don't think this PR poses any). Thanks for your patience 🙏 |
…dency
Removed `version` field from `[package]` section and from dependency
specification (`DepName = { .. version = "..." }`). Version is vestigial
and this is preparation for the version resolution hook to be
introduced.
…nknown field warnings
…ests in external-crates
|
I've rebased to main and removed version field from manifest files in |
|
thanks @kklas! |
gdanezis
pushed a commit
that referenced
this pull request
Dec 15, 2023
…fest sections (#15148) ## Description This PR removes the version field both from package section and dependency items in manifest (e.g. `SomeDep = { local = "../dep", version = "0.0.1" }`). Why is this safe to do? Well, the version field in the package section isn't used anywhere. It's not read anywhere in the codebase and thus not propagated through the toolchain. As for the version in dependency item, what it does it makes the dependency graph throw a version conflict when the same package is referenced with two different versions in the dependnecy graph. There are 3 types of dependencies (`Local`, `Git`, and `Custom`) out of which only `Local` and `Git` are used on Sui. Since referencing a different version of a dependency is already possible with git and local by specifying a different location, this version field isn't useful. I've also added `version` as a custom field in Sui hooks in order to make the `unknown field name found` warning go away when compiling packages with `sui move`. This should be removed once all `version` fields are removed from all manifests in this repo. This will be a breaking change for users running the old version of the toolchain so it's probably best to do that a bit later. This is part of the work to enable compiling against on-chain dependencies #14178. cc @amnn @rvantonder ## Test Plan Unit tests. Some tests (like the `diamond_problem_dep_dev_override_with_reg`) were using the `version` field to create a version conflict while using a package in the same location. I've refactored those tests to instead refer to the same package in different location. --- 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) - [ ] protocol change - [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 `Move.toml` files no longer require a `version` field in the `[package]` section and will warn at the presence of a `version` field in a `[dependency]` definition. Full PR description: #15148.
7 tasks
7 tasks
bmwill
pushed a commit
to move-language/move-sui
that referenced
this pull request
Apr 18, 2024
…fest sections (#15148)
## Description
This PR removes the version field both from package section and
dependency items in manifest (e.g. `SomeDep = { local = "../dep",
version = "0.0.1" }`).
Why is this safe to do?
Well, the version field in the package section isn't used anywhere. It's
not read anywhere in the codebase and thus not propagated through the
toolchain.
As for the version in dependency item, what it does it makes the
dependency graph throw a version conflict when the same package is
referenced with two different versions in the dependnecy graph. There
are 3 types of dependencies (`Local`, `Git`, and `Custom`) out of which
only `Local` and `Git` are used on Sui. Since referencing a different
version of a dependency is already possible with git and local by
specifying a different location, this version field isn't useful.
I've also added `version` as a custom field in Sui hooks in order to
make the `unknown field name found` warning go away when compiling
packages with `sui move`. This should be removed once all `version`
fields are removed from all manifests in this repo. This will be a
breaking change for users running the old version of the toolchain so
it's probably best to do that a bit later.
This is part of the work to enable compiling against on-chain
dependencies MystenLabs/sui#14178.
cc @amnn @rvantonder
## Test Plan
Unit tests.
Some tests (like the `diamond_problem_dep_dev_override_with_reg`) were
using the `version` field to create a version conflict while using a
package in the same location. I've refactored those tests to instead
refer to the same package in different location.
---
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)
- [ ] protocol change
- [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
`Move.toml` files no longer require a `version` field in the `[package]`
section and will warn at the presence of a `version` field in a
`[dependency]` definition. Full PR description:
MystenLabs/sui#15148.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR removes the version field both from package section and dependency items in manifest (e.g.
SomeDep = { local = "../dep", version = "0.0.1" }).Why is this safe to do?
Well, the version field in the package section isn't used anywhere. It's not read anywhere in the codebase and thus not propagated through the toolchain.
As for the version in dependency item, what it does it makes the dependency graph throw a version conflict when the same package is referenced with two different versions in the dependnecy graph. There are 3 types of dependencies (
Local,Git, andCustom) out of which onlyLocalandGitare used on Sui. Since referencing a different version of a dependency is already possible with git and local by specifying a different location, this version field isn't useful.I've also added
versionas a custom field in Sui hooks in order to make theunknown field name foundwarning go away when compiling packages withsui move. This should be removed once allversionfields are removed from all manifests in this repo. This will be a breaking change for users running the old version of the toolchain so it's probably best to do that a bit later.This is part of the work to enable compiling against on-chain dependencies #14178.
cc @amnn @rvantonder
Test Plan
Unit tests.
Some tests (like the
diamond_problem_dep_dev_override_with_reg) were using theversionfield to create a version conflict while using a package in the same location. I've refactored those tests to instead refer to the same package in different location.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)
Release notes
Move.tomlfiles no longer require aversionfield in the[package]section and produces a warning at the presence of aversionfield in a[dependency]definition. For the full description, see #15148.