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

cs2cs, cct, proj and geod: fflush(stdout) after each line to emit each result as … #2429

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

rouault
Copy link
Member

@rouault rouault commented Nov 17, 2020

…soon as it is produced

This is needed when working with pipes, when stdout is not an interactive terminal,
and thus the behaviour is to have it buffered as a regular file, whereas with
an interactive terminal, each newline character causes an implicit flush.

@kbevers
Copy link
Member

kbevers commented Nov 17, 2020

Does this not apply to proj and geod?

@rouault rouault changed the title cs2cs and cct: fflush(stdout) after each line to emit each result as … cs2cs, cct, proj and geod: fflush(stdout) after each line to emit each result as … Nov 17, 2020
@rouault
Copy link
Member Author

rouault commented Nov 17, 2020

Does this not apply to proj and geod?

good point. I've just patched them too

…h result as soon as it is produced

This is needed when working with pipes, when stdout is not an interactive terminal,
and thus the behaviour is to have it buffered as a regular file, whereas with
an interactive terminal, each newline character causes an implicit flush.
@rouault rouault merged commit c560b79 into OSGeo:master Nov 17, 2020
@PROJ-BOT
Copy link
Contributor

The backport to 7.2 failed:

The process 'git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-7.2 7.2
# Navigate to the new working tree
cd .worktrees/backport-7.2
# Create a new branch
git switch --create backport-2429-to-7.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick c560b7957664c32e2465e8425abaccc5a6b2607d
# Push it to GitHub
git push --set-upstream origin backport-2429-to-7.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-7.2

Then, create a pull request where the base branch is 7.2 and the compare/head branch is backport-2429-to-7.2.

@rouault
Copy link
Member Author

rouault commented Nov 17, 2020

manually backported

@busstoptaktik
Copy link
Member

This is needed when working with pipes, when stdout is not an interactive terminal

@rouault: Is this really needed? I have always done roundtrip-tests like this:

$ type foo

12 55 0 0
12 56 0 0
12 57 0 0

$ type foo | cct proj=utm zone=32 -- | cct -I proj=utm zone=32 --

 11.9999999994   55.0000000000        0.0000        0.0000
 11.9999999998   55.9999999996        0.0000        0.0000
 12.0000000000   56.9999999998        0.0000        0.0000

this works without any problems, even using cmd.exe: The second cct instance in the pipe blocks until the first autoflushes - either because buffer is full, or because it reaches eof. And in this example, stdout of the first cct instance is not an interactive terminal.

@rouault
Copy link
Member Author

rouault commented Nov 17, 2020

Is this really needed?

the issue of the reporter was that lines were not flushed as soon as they were emitted. Whole flushing when the buffer is full or the process is finished indeed worked before my change.

@busstoptaktik
Copy link
Member

So the problem occurs in a more specific situation, not detailed in the PR description.

I have not done any timing (and do not intend to), but it would be nice to have had some more context of the problem addressed, since flushing takes time (although probably not much, compared to the awfully slow formatting of floats, which in some cases dominates the cct run time).

I'm not against this PR - I just have a feeling it targets a very narrow use case, and hence, I think the rationale should have been more clear in the description (which I find misleading - or at least which seriously puzzled me. Blame it on my stupidity, if you wish).

@rouault
Copy link
Member Author

rouault commented Nov 17, 2020

Sorry for the short PR description. This is meant to address https://lists.osgeo.org/pipermail/proj/2020-November/009921.html

@busstoptaktik
Copy link
Member

Thanks - I hadn't seen that, but it was actually exactly the one use case I could imagine, when I tried to guess the rationale. I think it makes good sense to support this kind of work.

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.

None yet

4 participants