add riscv feature by default for benchmarks

[mordamax]

No description provided.

[athei]

Did you test that? I don't think all binaries have the riscv feature? I am not sure.

[mordamax]

Did you test that? I don't think all binaries have the riscv feature? I am not sure.

Do you mean rustc-rv32e-toolchain ? It's part of ci-unified image, so should work theoretically
i was testing it meanwhile - https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7431951
looks like it doesn't compile yes, but not sure what's the issue, I asked @xermicus in dm, but may be you could advice

[athei]

No. What I mean is that if you supply --features riscv the crate you are compiling needs to have this feature. I don't think it's the case for all the invocations you specified here.

[mordamax]

No. What I mean is that if you supply --features riscv the crate you are compiling needs to have this feature. I don't think it's the case for all the invocations you specified here.

Then I was probably confused with this statement paritytech/polkadot-sdk#5714 (comment)

I thought you can provide feature and if it doesn't exist anywhere it will skip or something.
Could you elaborate then what did you mean by Just always enabling the riscv feature when benchmarking is good enough ?

[athei]

Unfortunately, that is not how cargo works. The crate you are building has to have the feature or it will error out. This means only add the feature when building a node that has the feature. Only add the feature when building the package staging-node-cli.

We can add the feature to other nodes, too. But I don't understand the script here. How many different nodes it is building here for?

[ggwpez]

I think you can either:

• add --workspace, then it builds everything but the feature is enabled
• add --features pallet-revive/riscv or so, i think there is a syntax for this to enable a feature of a dependency. Dont know if it needs to be a direct dependency though.
• Add it to the node as Alex suggested

[athei]

The first point is a bit wasteful. The second will not work as it wouldn't pass the feature into the wasm build. For that it has to be defined on the runtime and that one passes it down.

I would say we just add the feature to the nodes (last point). But I have to understand which nodes are actually build by this script.

[athei]

tl;dr Only add the feature for the kitchensink node and parachain node. Remove for trappist and polkadot. I will do a PR do add this feature to the parachain node.

[athei]

paritytech/polkadot-sdk#5832

[athei]

Changed my mind. Let's only add it for the kitchensink node.

[athei]

Is this now adding riscv by default or does it allow to specify features?

[mordamax]

Is this now adding riscv by default or does it allow to specify features?

both ) it adds to dev/kitchensink, but also allows you to provide it --features=blabla

[athei]

Do we even need that since this is the deprecated approach and we can do everything with /cmd? Asking because eventually we will remove the riscv feature once we have an upstream toolchain.

[mordamax]

Do we even need that since this is the deprecated approach and we can do everything with /cmd? Asking because eventually we will remove the riscv feature once we have an upstream toolchain.

good point. I think it's fine, as I hope we anyway deprecate this bot as soon as we confirm /cmd works well for all use-cases, and we just archive this repo