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

Add custom rust toolchain support #22

Closed
wants to merge 3 commits into from
Closed

Conversation

fewensa
Copy link
Contributor

@fewensa fewensa commented Jul 28, 2023

README.md Outdated Show resolved Hide resolved
@chevdor
Copy link
Owner

chevdor commented Jul 28, 2023

Custom RUSTC_VERSION

Picking the RUSTC_VERSION is not implemented in srtool-actions on purpose.

The idea of fetching the canonical version from https://raw.githubusercontent.com/${{ inputs.image }}/master/RUSTC_VERSION is to ensure that all users use the same canonicial version so that any user can check any runtime without having to worry about what srtool to use. It has its short-comings and having the information of the RUSTC_VERSION in the runtime itself is an option I am considering.

At the moment, If you start building your runtime with something else than the canocial version, the checks will fail and the users of your runtime will wonder what the problem is.

Could you explain what your use case is ?

@fewensa
Copy link
Contributor Author

fewensa commented Jul 29, 2023

Two reasons.

  1. not all like substrate projects use same rust toolchain with substrate/polkadot
  2. because of you published docker image tag named semantic style version. but some project use date named version number. https://github.com/darwinia-network/darwinia/blob/3b4dbf996945f1b5237fd5834c0cc38cbc446ceb/rust-toolchain.toml#L2

and srtool build in runtime directory, so can not read root directory rust-toolchain.toml file do override rust toolchain (I tested it's not work). so hopefully srtool can set rustc_version

@chevdor
Copy link
Owner

chevdor commented Jul 31, 2023

not all like substrate projects use same rust toolchain with substrate/polkadot

Ok, fair enough. substrate/polkadot use the latest stable. it should be a goal to get there. It is possible since recently and you should really aim for that.

because of you published docker image tag named semantic style version

This is mainly for users to find they way around since currently, they will not have an easy way to know when you built your runtimes.

and srtool build in runtime directory, so can not read root directory rust-toolchain.toml file do override rust toolchain (I tested it's not work)

This is blocked on purpose yes

Let me think how we can solve your problem without comprising too much for the users.

@fewensa fewensa reopened this Aug 1, 2023
@fewensa fewensa changed the title Make changes follow upstream Add custom rust toolchain support Aug 1, 2023
@AurevoirXavier
Copy link

This is blocked on purpose yes

Let me think how we can solve your problem without comprising too much for the users.

Currently, I am using a soft link to resolve this issue.

@AurevoirXavier
Copy link

Hey @chevdor, any feedback?

@chevdor
Copy link
Owner

chevdor commented Oct 20, 2023

I am not completely happy with the raw solution as it is really a foot gun.

Nothing against the PR, my main concern is related to users trying to verify runtimes. With this PR in, once the feature is used to build runtimes, those will appear "invalid" unless users can find and use the right version.

I have some ideas how to fix that cleanly but that still requires testing.

@AurevoirXavier
Copy link

I am not completely happy with the raw solution as it is really a foot gun.

Sorry, I mean this.

Currently, I am using a soft link to resolve this issue.

I think maybe we can close this PR.

@chevdor
Copy link
Owner

chevdor commented Oct 20, 2023

It would be nice to hear from @fewensa whether your "outlaw" solution 😄 is an acceptable workaround.

@fewensa
Copy link
Contributor Author

fewensa commented Oct 23, 2023

I still hope to be implemented by srtool-actions, because this is more general. of course softlink it's works for me

@fewensa fewensa closed this Sep 25, 2024
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