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

Handle pipes #4625

Merged
merged 1 commit into from
Mar 1, 2023
Merged

Handle pipes #4625

merged 1 commit into from
Mar 1, 2023

Conversation

LudvikGalois
Copy link
Contributor

Fixes #4235

@LudvikGalois LudvikGalois force-pushed the ludvikgalois/handle-pipes branch from 3aea0f3 to 8cf789d Compare November 15, 2022 05:39
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Sorry, I'm a bit confused. How does this work?

cardano-api/src/Cardano/Api/Utils.hs Show resolved Hide resolved
cardano-api/src/Cardano/Api.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs Outdated Show resolved Hide resolved
@LudvikGalois
Copy link
Contributor Author

Sorry, I'm a bit confused. How does this work?

In the normal course of operation, we sometimes read files multiple times. This is somewhat unexpected for a user, and doesn't work if the file isn't a real file, but a pipe, since after the contents have been read there's nothing to left to read.
FileOrPipe is a reference to a file-like thing. readFileOrPipe reads the file into memory (this is not ideal, but otherwise we have to deal with lazy IO, which can be a headache and in general these are small files) and once it's done checks if the file is a pipe or not. If it's a pipe, it saves the contents in an IORef so that will be used on subsequent reading attempts, otherwise it throws it away.
What changes is that some functions which used to take filenames, now take FileOrPipes and readFileOrPipe is used instead of readFile.

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Nov 18, 2022

Sorry, I'm a bit confused. How does this work?

In the normal course of operation, we sometimes read files multiple times. This is somewhat unexpected for a user, and doesn't work if the file isn't a real file, but a pipe, since after the contents have been read there's nothing to left to read. FileOrPipe is a reference to a file-like thing. readFileOrPipe reads the file into memory (this is not ideal, but otherwise we have to deal with lazy IO, which can be a headache and in general these are small files) and once it's done checks if the file is a pipe or not. If it's a pipe, it saves the contents in an IORef so that will be used on subsequent reading attempts, otherwise it throws it away. What changes is that some functions which used to take filenames, now take FileOrPipes and readFileOrPipe is used instead of readFile.

Thanks for the explanation. Can you add a summary as a comment on top of readFileOrPipe? Happy to approve once the other comments are addressed. We also need @saratomaz to confirm this fix is working.

@saratomaz
Copy link

Solved

txFileJSON=$(cat <<-EOF
{
  "type": "Unwitnessed Tx BabbageEra",
  "description": "Ledger Cddl Format",
  "cborHex": "84a300818258208a5a31ae52cdc140b1c532c47d5cb3c8dbc6c9b7a4aa537ed9e3543ea94cfad1000183a200581d60768d2bcd29a84ae015d96118e3285bc0cbd399ae16039ad471c8a806011a001e8480a200581d60768d2bcd29a84ae015d96118e3285bc0cbd399ae16039ad471c8a806011a002625a0a200581d600ac4d3db1163e7884959c0317aa19263a236db113fa0fbefbd49b335011a00958d31021a0003094fa0f5f6"
}
EOF
)

cardano-cli transaction txid --tx-file <(echo "${txFileJSON}")
a2e58d1b59d8c06535a1134f0022b1c77b5dc679248f635197546b6855830730

@LudvikGalois LudvikGalois force-pushed the ludvikgalois/handle-pipes branch 2 times, most recently from a8ea22e to b8282f9 Compare November 22, 2022 20:58
@LudvikGalois LudvikGalois force-pushed the ludvikgalois/handle-pipes branch 3 times, most recently from 0d4372b to 36704ea Compare December 28, 2022 01:16
@soltanoff
Copy link

Hi, @LudvikGalois, @saratomaz

What about the status of this PR? I need it very much.

Thank you!

@soltanoff
Copy link

@erikd, @Jimbo4350 jFYI.

@LudvikGalois
Copy link
Contributor Author

Hi, @LudvikGalois, @saratomaz

What about the status of this PR? I need it very much.

Thank you!

I no longer work at IOG, and @Jimbo4350 (who this has likely passed to) is probably drowning in work, so this may not be addressed immediately.

However, it is almost "ready to merge". After a rebase, everything apart from the test should be fine. As for the test, the current incarnation of the test doesn't actually need a testnet, so it should move from the testnet project back into the CLI. It also needs some tweaking so it'll compile under windows (it currently fails to compile on Windows due to unused imports, because the test itself does some unix specific things, so the windows implementation of the test is a stub which is then skipped).

@Jimbo4350 Jimbo4350 force-pushed the ludvikgalois/handle-pipes branch from 36704ea to fac15ab Compare February 7, 2023 17:58
@Jimbo4350
Copy link
Contributor

Hi, @LudvikGalois, @saratomaz

What about the status of this PR? I need it very much.

Thank you!

I'm working on this to get it merged this week.

@soltanoff
Copy link

@LudvikGalois, oh, I got it. Thank you for information.

@Jimbo4350, I’ll waiting for it, thanks a lot 🙏.

@Jimbo4350 Jimbo4350 force-pushed the ludvikgalois/handle-pipes branch 2 times, most recently from ade6d4b to 38c2f4c Compare February 8, 2023 14:08
@saratomaz
Copy link

@Jimbo4350 let me know if you want me to test it again

@Jimbo4350 Jimbo4350 force-pushed the ludvikgalois/handle-pipes branch 3 times, most recently from a31e698 to 6865cb7 Compare February 8, 2023 16:21
@newhoggy newhoggy force-pushed the ludvikgalois/handle-pipes branch 5 times, most recently from 471a73c to e52ac45 Compare February 13, 2023 11:44
@newhoggy
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 13, 2023
4625: Handle pipes r=newhoggy a=LudvikGalois

Fixes #4235

Co-authored-by: Robert 'Probie' Offner <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 13, 2023

Build failed:

@newhoggy newhoggy force-pushed the ludvikgalois/handle-pipes branch from e52ac45 to cef94de Compare February 14, 2023 21:06
@newhoggy
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 14, 2023
4625: Handle pipes r=newhoggy a=LudvikGalois

Fixes #4235

Co-authored-by: Robert 'Probie' Offner <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 15, 2023

Timed out.

@newhoggy newhoggy force-pushed the ludvikgalois/handle-pipes branch from cef94de to cc2853a Compare February 20, 2023 05:37
@newhoggy
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 20, 2023
4625: Handle pipes r=newhoggy a=LudvikGalois

Fixes #4235

Co-authored-by: Robert 'Probie' Offner <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 20, 2023

Timed out.

@Jimbo4350 Jimbo4350 force-pushed the ludvikgalois/handle-pipes branch from cc2853a to ac73752 Compare February 28, 2023 16:30
@Jimbo4350
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 28, 2023
4625: Handle pipes r=Jimbo4350 a=LudvikGalois

Fixes #4235

4682: Export `fromShelleyBasedScript` from Cardano.Api r=Jimbo4350 a=eyeinsky

New PR based off of a branch in this repo. Old PR here #4386

Co-authored-by: Robert 'Probie' Offner <[email protected]>
Co-authored-by: Markus Läll <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 28, 2023

This PR was included in a batch that timed out, it will be automatically retried

iohk-bors bot added a commit that referenced this pull request Feb 28, 2023
4625: Handle pipes r=Jimbo4350 a=LudvikGalois

Fixes #4235

4908: Added features to tracing r=jutaro a=jutaro

* Generated docu shows tracers, tracers with metrics, silent tracers according to current configuration
* Trace message shows tracers with metrics and silent tracers

Co-authored-by: Robert 'Probie' Offner <[email protected]>
Co-authored-by: Yupanqui <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 1, 2023

This PR was included in a batch that timed out, it will be automatically retried

Fixes #4235
Co-authored-by: John Ky <[email protected]>
@newhoggy newhoggy force-pushed the ludvikgalois/handle-pipes branch from ac73752 to 71b5318 Compare March 1, 2023 02:11
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 1, 2023

Canceled.

@newhoggy
Copy link
Contributor

newhoggy commented Mar 1, 2023

bors r+

@newhoggy newhoggy self-assigned this Mar 1, 2023
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 1, 2023

Build succeeded:

@iohk-bors iohk-bors bot merged commit 8040dce into master Mar 1, 2023
@iohk-bors iohk-bors bot deleted the ludvikgalois/handle-pipes branch March 1, 2023 04:06
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.

[BUG] - CLI is not allowing process substitution on certain commands
5 participants