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

Update swanky check #195

Closed
wants to merge 16 commits into from
Closed

Update swanky check #195

wants to merge 16 commits into from

Conversation

prxgr4mm3r
Copy link
Collaborator

No description provided.

Copy link
Contributor

@ipapandinas ipapandinas left a comment

Choose a reason for hiding this comment

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

Checkbox 4 from the original issue #114 is not fully completed:

fetch and store the dependency table on init and check use

We should update node.supportedInk version in swanky config when performing the checkCliDependencies task in the init command. You can make and helper performing the semver.gt against our cargo-contract dependencies map like you did in "Verify ink version" from the check command. Then update the node.supportedInk version in swanky config accordingly.

src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Show resolved Hide resolved
@ipapandinas ipapandinas changed the base branch from feature/node-version to master January 18, 2024 10:59
@ipapandinas ipapandinas changed the base branch from master to feature/node-version January 18, 2024 10:59
@prxgr4mm3r
Copy link
Collaborator Author

prxgr4mm3r commented Jan 26, 2024

Checkbox 4 from the original issue #114 is not fully completed:

fetch and store the dependency table on init and check use

We should update node.supportedInk version in swanky config when performing the checkCliDependencies task in the init command. You can make and helper performing the semver.gt against our cargo-contract dependencies map like you did in "Verify ink version" from the check command. Then update the node.supportedInk version in swanky config accordingly.

@ipapandinas Sorry, I don't understand properly what you mean by updating node.supportedInk. I thought that it should be set initially in nodeInfo for every SwankyNode version. Can you explain what factors affect the update of the node.supportedInk?

Copy link
Contributor

@ipapandinas ipapandinas left a comment

Choose a reason for hiding this comment

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

@prxgr4mm3r you are right node.supportedInk has to be set in nodeInfo and remains static.

Also, I reviewed the UX of the check command, the following tasks have to be perform in this order:

  1. "Check OS"
  2. "Check Rust"
  3. "Check cargo"
  4. "Check cargo nightly"
  5. "Check cargo dylint"
  6. "Check cargo-contract"
  7. "Read ink dependencies"
  8. "Verify ink version"
    (skipped if no swanky node is installed in swankyConfig)
    ensure compatibility with swanky node: the supportedInk should be greater or equal to the read ink version in step 7
    if not ensured => throw a WARN: Version of ${inkPackage} (${version}) in ${contract} is higher than supported ink version (${supportedInk})
  9. "Verify cargo contract version"
    (skipped if no cargo contract installed => tools.cargoContract === null)
    ensure cargo contract version is greater or equal to the minimal version for the read ink version in step 7
  10. "Check for missing tools"
    print a console.error([ERROR] ${error}) for each missing tool

So "Check swanky node" becomes useless.

Also additional enhancements:

Try to print versions next to task success messages like this:

✔ Check OS: 'darwin-arm64'
✔ Check Rust: '1.72.1'
✔ Check cargo: '1.72.1'
...
You can extracted them using regex like you did for cargo contract

Also try to throw a WARN instead of error for dylint

✖ Cargo dylint is not installed!

Also checkbox 2 is not implemented

add optional file output

It can be an optional flag --print and the output file should look like this:

{
  "platform": "darwin",
  "architecture": "arm64",
  "tools": {
    "rust": "1.72.1",
    "cargo": "1.72.1",
    "cargoNightly": "1.77.0",
    "cargoContract": "4.0.0-rc.1"
  },
  "missingTools": [],
  "contracts": { "flipper": { "ink": "4.2.1" } },
  "swankyNode": null
}

src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/lib/cargoContractInfo.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
@vsofiya vsofiya mentioned this pull request Jan 31, 2024
7 tasks
src/lib/cargoContractInfo.ts Show resolved Hide resolved
src/commands/check/index.ts Outdated Show resolved Hide resolved
src/commands/check/index.ts Show resolved Hide resolved
@pmikolajczyk41 pmikolajczyk41 mentioned this pull request Feb 6, 2024
Base automatically changed from feature/node-version to ink-devhub-1 February 12, 2024 10:33
@pmikolajczyk41
Copy link
Member

why was it closed instead of merged? @ipapandinas

@ipapandinas
Copy link
Contributor

I created a single PR #205 to address the overlap between features and to prepare for the upcoming release. I closed this particular one due to significant conflicts. @pmikolajczyk41

@pmikolajczyk41
Copy link
Member

@ipapandinas okay, I was just confused that the other 3 PRs were merged, so I wanted to ensure - thanks!

@ipapandinas ipapandinas deleted the feature/update-check branch February 27, 2024 19:37
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