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.

Sign up for GitHub

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

Jump to bottom

chore: Update README prerequisites #9295

Merged
merged 3 commits into from
Apr 29, 2024
Merged

chore: Update README prerequisites #9295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
380c068
chore: Update README prerequisites
gibson042 Apr 26, 2024
c8a79e1
chore(golang/cosmos): Replace custom cutPrefix with stdlib bytes.CutP…
gibson042 Apr 26, 2024
9f934bf
chore: Require README updates when bumping Node.js version support
gibson042 Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ to use.
## Prerequisites

* Git
* Node.js LTS (version 16.13.0 or higher)
* Go ^1.20.2
* Node.js ^18.12 or ^20.9
Copy link
Member

Choose a reason for hiding this comment

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

the policy is Node LTS, whatever that may be. let's leave specific version numbers out of the README.

though consider linking from here to https://github.com/Agoric/agoric-sdk/blob/master/docs/node-version.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, does that make sense in a list that is specific with respect to other software? I think instead it makes sense to have the README be the source of truth and docs/node-version.md to include instructions for updating it in accordance with Node.js LTS policy. We shouldn't expect someone to be able to use a Node.js version that we haven't actively vetted, even if it is LTS.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @gibson042, there is a difference between policy, and what is actually tested. The goal of this section is also to be referenced from release notes by permalink, so we should have an explicit statement of what versions are explicitly supported and tested.

Copy link
Member

Choose a reason for hiding this comment

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

I think instead it makes sense to have the README be the source of truth and docs/node-version.md to include instructions for updating it in accordance with Node.js LTS policy

I'm not concerned with which doc is the source of truth, but that any doc is rather than CI or package.json engines which is the actual truth about what is supported. I'm weary of multiple sources of "truth" and the likelihood they get out so sync.

Take that input fwiw. No objections to the PR as is.

* we generally support the latest LTS release: use [nvm](https://github.com/nvm-sh/nvm) to keep your local system up-to-date
* Yarn (`npm install -g yarn`)
* gcc-10 or newer, or a compiler with `__has_builtin()`
* gcc >=10, clang >=10, or another compiler with `__has_builtin()`
Copy link
Member Author

Choose a reason for hiding this comment

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

clang 10 is very old, but consistent with our releases.


Any version of Yarn will do: the `.yarnrc` file should ensure that all
commands use the specific checked-in version of Yarn (stored in
Expand Down
1 change: 1 addition & 0 deletions docs/node-version.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ When a new version becomes Active LTS:
- [ ] update the .node-version hint to use it
- [ ] update Node.js test ranges to remove the EOLed version and add the new LTS
- [ ] update package.json engines to allow the two LTS versions
- [ ] update README.md to document the new supported versions
12 changes: 2 additions & 10 deletions golang/cosmos/x/vstorage/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,6 @@ var _ ChangeManager = (*BatchingChangeManager)(nil)
// 2 ** 256 - 1
var MaxSDKInt = sdk.NewIntFromBigInt(new(big.Int).Sub(new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), big.NewInt(1)))

// TODO: Use bytes.CutPrefix once we can rely upon go >= 1.20.
func cutPrefix(s, prefix []byte) (after []byte, found bool) {
if !bytes.HasPrefix(s, prefix) {
return s, false
}
return s[len(prefix):], true
}

// Keeper maintains the link to data storage and exposes getter/setter methods
// for the various parts of the state machine
type Keeper struct {
Expand Down Expand Up @@ -164,7 +156,7 @@ func (k Keeper) ExportStorageFromPrefix(ctx sdk.Context, pathPrefix string) []*t
if !strings.HasPrefix(path, pathPrefix) {
continue
}
value, hasPrefix := cutPrefix(rawValue, types.EncodedDataPrefix)
value, hasPrefix := bytes.CutPrefix(rawValue, types.EncodedDataPrefix)
if !hasPrefix {
panic(fmt.Errorf("value at path %q starts with unexpected prefix", path))
}
Expand Down Expand Up @@ -266,7 +258,7 @@ func (k Keeper) GetEntry(ctx sdk.Context, path string) agoric.KVEntry {
if bytes.Equal(rawValue, types.EncodedNoDataValue) {
return agoric.NewKVEntryWithNoValue(path)
}
value, hasPrefix := cutPrefix(rawValue, types.EncodedDataPrefix)
value, hasPrefix := bytes.CutPrefix(rawValue, types.EncodedDataPrefix)
if !hasPrefix {
panic(fmt.Errorf("value at path %q starts with unexpected prefix", path))
}
Expand Down
Loading