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

Address dev branch toolchain issues. #149

Merged
merged 13 commits into from
Apr 19, 2024

Conversation

zacshowa
Copy link
Contributor

@zacshowa zacshowa commented Apr 15, 2024

Changes in this PR

  1. This PR adds a rust-toolchain.rs to the repo, pinning the rust toolchain to the same dev in casper-node.

    • To achieve this, the PR pins 2 clap dependencies to versions that compile on the previously mentioned rust-toolchain.
  2. Additionally, this PR removes a dependency on vergen in the build.rs build script.

    • This functionality is maintained by replacing vergen with standard library functions to ask Cargo to populate an env var during the build lifecycle that the client reads while constructing its version string.

Zach Showalter added 6 commits April 15, 2024 13:52
…hort git commit hash to the clients version string

Signed-off-by: Zach Showalter <[email protected]>
…ets the `GIT_SHA_SHORT` env var during compile time so the client may use `env!` macros to populate the version string with a short git commit hash.

Signed-off-by: Zach Showalter <[email protected]>
…. (Using only one colon vs two for `cargo:rustc-env`)

Signed-off-by: Zach Showalter <[email protected]>
Signed-off-by: Zach Showalter <[email protected]>
Cargo.toml Outdated
@@ -51,7 +51,6 @@ uint = "0.9.5"
tempfile = "3.8.1"

[build-dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you can remove [build-dependencies] now.

Are you really really sure you have to fix clap ?

Copy link
Contributor

Choose a reason for hiding this comment

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

At least I observed issues where clap_lex (and likely other clap packages) specified a minimum Rust version (the is already on 1.74).

You can see that clap 4.4.18 fixes it at 1.70, whereas 4.5.0 already requires 1.74.

We could consider something less strict like <4.5.0 instead of =... though?

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 that the more liberal we can be with the versioning the better. I don't know what the SOP is for moving the toolchain version forward on the node. That is, when should we be moving the versioning forward vs leaving it the same.

Seeing as we currently don't use any of these cutting edge clap features, I don't see any reason to increment the toolchain version as we don't have a need to do so on the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it turns out the best I can do on this is pinning clap to ~4.4 and putting clap_complete at <4.5.0.

Setting clap at <4.5, for some esoteric reason, results in issues with clap_lex specifying Rust version 1.74

@marc-casperlabs
Copy link
Contributor

Looks good, although consider the slightly more liberal version fix. @zacshowa I think getting rid of vergen might also be something to do on the main casper-node, what do you think?

cc @rafal-ch @alsrdn @jacek-casper

@zacshowa
Copy link
Contributor Author

I think getting rid of vergen might also be something to do on the main casper-node

I agree, It's functionality could easily be replaced with standard library functions and calls to git. This would reduce the future risk of transitive dependency security issues at the cost of a small effort.

Copy link
Contributor

@marc-casperlabs marc-casperlabs left a comment

Choose a reason for hiding this comment

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

Looks good!

@zacshowa zacshowa merged commit 6b61f16 into casper-ecosystem:dev Apr 19, 2024
1 check passed
gRoussac added a commit to gRoussac/casper-client-rs that referenced this pull request Jul 17, 2024
gRoussac added a commit that referenced this pull request Aug 28, 2024
* Add feat-track-node-2.0 to CI

* Port from dev to feat-track-2.0 of #149 #175 Part of #144
Enable CI CD check on feat-track-2.0

* clippy/test/build with no default features not yet working

* Clippy

* Commenting out ci/cd for --no-default-features
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.

4 participants