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

Stop using shell output redirects #1515

Closed

Conversation

JoshOrndorff
Copy link

@JoshOrndorff JoshOrndorff commented Nov 14, 2023

This tiny PR changes the way that genesis wasm and head data are generated very slightly.

# Old way
collator-node export-genesis-wasm > some-wasm-file

# New way
collator-node export-genesis-wasm some-wasm-file

In the old days, using the shell redirect was the only way to get the file you wanted. But later cumulus added the ability to provide an output file argument and just fallback to printing directly to stdout.

I prefer using the argument because sometimes I find myself needing to print debugging output when actually generating that data, and I don't want my debugging output being redirected to the generated file. (As a concrete example, here is a current PR where I'm having this problem, and it has happened in the past too.)

…pidly called state; I'll fix that in cumulus soon too)
@wirednkod
Copy link
Contributor

Thank you @JoshOrndorff for this. There are more places where the outputs redirect is used and I think a more careful search and discovery of those is needed. I agree with the approach ( cc @pepoviola what do you think), but all of those should be in the same PR IMO.
Some examples:

https://github.com/paritytech/zombienet/blob/main/javascript/packages/orchestrator/src/paras.ts#L239-L242
https://github.com/paritytech/zombienet/blob/main/javascript/packages/orchestrator/src/paras.ts#L259

@JoshOrndorff
Copy link
Author

Are there build instructions anywhere so that I can build zombienet myself from my branch? It is hard to contribute to it when I can only wait for a release to test my changes.

@pepoviola
Copy link
Collaborator

Are there build instructions anywhere so that I can build zombienet myself from my branch? It is hard to contribute to it when I can only wait for a release to test my changes.

Hey @JoshOrndorff, sorry about the delay. I saw in your pr that you are now using eprintln and those lines will not be redirected to the file with the current shell redirection > . Is this the case?

But, as you suggest using the new method could be better since others that use println and are hit by the same issue.
Related to the build, here are the steps to build from source using node.js.

Thanks! and again sorry about the delayed answer.

@pepoviola
Copy link
Collaborator

Hey @JoshOrndorff, Thanks for open this pr but I think we will keep the shell redirect since works across cumulus/adder/undying collators. As I said before if you want to print without goes to the file you can use eprintln to print to stderr (as you made in your pr :)).

Again, thanks for your feedback and contribution!! 🙌

@pepoviola pepoviola closed this Nov 16, 2023
@JoshOrndorff
Copy link
Author

JoshOrndorff commented Nov 16, 2023

Needing to use eprintln is one more unintuitive workround to frustrate learners.

If you don't want this PR, that's your call. Each little annoyance that you don't fix is a more reason to find/make/hope for a better tool.

@pepoviola
Copy link
Collaborator

Needing to use eprintln is one more unintuitive workround to frustrate learners.

If you don't want this PR, that's your call. Each little annoyance that you don't fix is a more reason to find/make/hope for a better tool.

Hey @JoshOrndorff, I understand what you say about eprintln and I'm sorry that you find this annoyance. Let me re-work a little bit your pr to see how much overhead we need to support this in both cumulus and non-cumulus based collators. I opened this issue to track #1520. Again, thanks for your feedback and to help us making a better tool.

Thx!

@JoshOrndorff
Copy link
Author

Actually, this is even more important than I realized. There is a bug in Substrate Node Template for a while now where it occasionally prints Essential Task Failed when generating the specs or genesis data. The error is "harmless" in the sense that it doesn't mess up the generated output data. Unless you use the shell redirect.

Although I guess you could argue as well that the Essential Task Failed should be logged to stderr. I actually would agree with that point.

But still, this seems like a footgun that is worth avoiding.

And also, just for the record, my collator is exactly the stock cumulus collator. The thing that is unusual about it is that it doesn't use FRAME.

@pepoviola
Copy link
Collaborator

Hi @JoshOrndorff, thanks for pointing this issue. In a second thought, This improvement make total sense and I will add the fixes for other collators. Then in a follow up pr I will dig the same issue for building the spec.
Thanks!, and sorry about the noice.

@pepoviola
Copy link
Collaborator

Hi @JoshOrndorff, this is already fixed and included in the latest version v1.3.82.
Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants