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

Investigate: What dependencies should be made optional #507

Closed
clangenb opened this issue Apr 1, 2023 · 11 comments · Fixed by #522
Closed

Investigate: What dependencies should be made optional #507

clangenb opened this issue Apr 1, 2023 · 11 comments · Fixed by #522
Assignees
Labels
F7-enhancement Enhances an already existing functionality Q5-involved Will take some time to fix this

Comments

@clangenb
Copy link
Collaborator

clangenb commented Apr 1, 2023

Like contracts, staking and rustls stuff.

This will remove quite some dependencies in case they are not needed.

@haerdib
Copy link
Contributor

haerdib commented Apr 3, 2023

Could look similar to staking xt:

staking-xt = ["std"]

@haerdib haerdib changed the title Make contracts, staking, rustls stuff optional. Investigate: What dependencies should be made optional Apr 3, 2023
@haerdib
Copy link
Contributor

haerdib commented Apr 4, 2023

Should also include:

  • what feature flags
  • internal dependencies - should all of them always be forwarded?

@haerdib haerdib added F7-enhancement Enhances an already existing functionality Q5-involved Will take some time to fix this labels Apr 4, 2023
@Niederb
Copy link
Contributor

Niederb commented Apr 4, 2023

For following crates take the longest to compile: wasmparser, wasmtime, wasmparser-nostd, sp-state-machine and wasmi

@clangenb
Copy link
Collaborator Author

clangenb commented Apr 4, 2023

If I am not mistaken, they are exclusively used by the pallet-contracts.

@Niederb
Copy link
Contributor

Niederb commented Apr 4, 2023

Unfortunately sp-wasm-interface has a dependency on wasmi as well (at least in std mode). See this issue/comment #272 (comment)

@clangenb
Copy link
Collaborator Author

clangenb commented Apr 4, 2023

Hmm, I see. However, we did not have these dependencies before in the worker: https://github.com/integritee-network/worker/pull/1263/files

Which intrigues me now.

@Niederb
Copy link
Contributor

Niederb commented Apr 4, 2023

Oh, that is interesting indeed. I will do a bit more digging as it would be nice to get rid of them not only for compilation time but also to allow better wasm support.

@clangenb
Copy link
Collaborator Author

clangenb commented Apr 4, 2023

Cool, sounds great!

@Niederb
Copy link
Contributor

Niederb commented Apr 5, 2023

@clangenb Hmm, I checked and most of the dependencies are not really new if I look at:
https://github.com/integritee-network/worker/blob/945571ae07f6efdebb7cf67b21e5e31b1b4b6ee4/Cargo.lock
Only wasmparser-nostd is completely new wasmi is now duplicated with different version numbers.
One thing I noticed: In the worker there are some dependencies to the substrate-api-client without default-features = false. This now activates the jsonrpsee-client feature which is not what you might want/need. I think disabling the default features could already help a bit with your dependencies. Could you check what happens?

@clangenb
Copy link
Collaborator Author

clangenb commented Apr 5, 2023

Nice, this is what happens: https://github.com/integritee-network/worker/pull/1271/files.

So nothing regarding the wasm stuff, but still a nice side-effect.

Yeah, I only checked the diff I must admit, sorry for that. :)

@Niederb
Copy link
Contributor

Niederb commented Apr 5, 2023

Great. I was not really expecting to see changes regarding wasm, but at least one of the rustls dependencies is gone 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F7-enhancement Enhances an already existing functionality Q5-involved Will take some time to fix this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants