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.

Sign up for GitHub

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

Jump to bottom

[BUG] cython command prints stdout-type information to stderr #5504

Closed
scoder opened this issue Jun 28, 2023 · 8 comments · Fixed by #5505
Closed

[BUG] cython command prints stdout-type information to stderr #5504

scoder opened this issue Jun 28, 2023 · 8 comments · Fixed by #5505
Labels
defect Error Reporting

Comments

@scoder
Copy link
Contributor

scoder commented Jun 28, 2023

Describe the bug

As reported on the mailing list, the cython command prints things to stderr that should go to stdout.

Code to reproduce the behaviour:

>>> from subprocess import run
>>> res = run("cython --version", capture_output=True, shell=True)
>>> err = res.stderr.splitlines()
>>> print(err)
[b'Cython version 0.29.35']
>>> out = res.stdout.splitlines()
>>> print(out)
>>> []

Expected behaviour

No response

OS

No response

Python version

No response

Cython version

0.29.35, probably also master

Additional context

No response
@scoder
Copy link
Contributor Author

scoder commented Jun 28, 2023

ISTM that 3.0 would be the right time to fix this. Not sure if it's a) important enough to b) still change it at this point.

@mattip mattip mentioned this issue Jun 29, 2023
Merged
scoder pushed a commit that referenced this issue Jun 29, 2023
@mattip
Print --version to stdout (GH-5505)
4f4e91b 
Closes #5504
@mattip
Copy link
Contributor

mattip commented Jul 12, 2023

It turns out fixing this broke meson, see scipy/scipy#18865. Two thoughts

  • I wonder if CMake will be hit by this as well.
  • cython should add a minimal meson/CMake build project to its CI. I am currently updating the NumPy test to build a c-extension using Cython and the numpy.random.*.pxd files, so something along the lines of this could also work for cython

@mattip mattip mentioned this issue Jul 12, 2023
Closed
@scoder
Copy link
Contributor Author

scoder commented Jul 12, 2023

I could live with the ugliness of writing the version to both stdout and stderr in 3.0 and then removing the stderr print after changing the master branch to 3.1-pre. That way, things would keep working for another while and tools would get time to adapt. (Although they probably wouldn't until things start failing again…)

Or, we revert the whole thing. We've been living with the stderr version output for 20 years. It's unexpected, and wouldn't be done like this if it wasn't there already, but it's not at all harmful.

@mattip
Copy link
Contributor

mattip commented Jul 12, 2023

cc @eli-schwartz, I think meson is going to make a quick-fix release to deal with this.

@scoder
Copy link
Contributor Author

scoder commented Jul 12, 2023

That would be great. But as you also noted in the scipy ticket, it's probably not the only tool that wants to read the version.

scoder added a commit that referenced this issue Jul 12, 2023
@scoder
Write the version to stderr as before, but only if stderr is actually…
36a3702 
… captured and not just printed alongside with stdout.

See #5504
@scoder
Copy link
Contributor Author

scoder commented Jul 12, 2023

I pushed a change that writes the version to stderr as well, unless that's a TTY. That way, we won't print it twice, at least, but would still make it available to tools that read from Cython's stderr.

@da-woods
Copy link
Contributor

Are we worried that this blocks too many people from testing the release candidate? (i.e. does this justify needing a second release candidate?) Or has Meson successfully worked round it too?

@jarrodmillman jarrodmillman mentioned this issue Jul 12, 2023
Merged
eli-schwartz added a commit to eli-schwartz/meson that referenced this issue Jul 12, 2023
@eli-schwartz
compilers: detect cython version on stdout for newer cython versions
e2ce53b 
Cython historically, when asked to print the version and exit
successfully, would do so on stderr, which is weird and inconsistent.
Recently, it fixed this UX bug by printing on stdout instead:

cython/cython#5504

This then broke meson detection because we assumed it was on stderr due
to historically being there:

scipy/scipy#18865

Cython is right, and shouldn't have to revert this reasonable change for
backwards compatibility. Instead, check both.
@eli-schwartz eli-schwartz mentioned this issue Jul 12, 2023
Merged
@eli-schwartz
Copy link
Contributor

eli-schwartz commented Jul 12, 2023 •
edited

cc @eli-schwartz, I think meson is going to make a quick-fix release to deal with this.

To clarify, meson is currently running its own release candidate 3 and the final release is expected on Sunday if all goes well. The fix is merged to meson git master, and will be part of that feature release.

Are we worried that this blocks too many people from testing the release candidate? (i.e. does this justify needing a second release candidate?) Or has Meson successfully worked round it too?

When is cython hoping to release the final release?

eli-schwartz added a commit to eli-schwartz/cython that referenced this issue Jul 28, 2023
@eli-schwartz
Only write the version to stderr, if it's a separate file descriptor …
0532a7f 
…from stdout

At least one project tries to detect the Cython version by redirecting stderr to
stdout and capturing it. This is done in pure POSIX shell, so it probably seemed
like the simple and obvious solution for a less capable programming language
given that no output at all was expected on stdout.

But the result is that the version number appears twice, and then gets misparsed
and ends up triggering bad assumptions in the code running cython.

It turns out that it's pretty easy to just print once, though. Detect when
stdout and stderr are redirected to the same location, and only print once.

See cython#5504
Fixes https://bugs.gentoo.org/911333
@eli-schwartz eli-schwartz mentioned this issue Jul 28, 2023
Merged
scoder pushed a commit that referenced this issue Jul 28, 2023
@eli-schwartz
Only write the version to stderr, if it's a separate file descriptor …
ca69b35 
…from stdout (GH-5572)

At least one project tries to detect the Cython version by redirecting stderr to
stdout and capturing it. This is done in pure POSIX shell, so it probably seemed
like the simple and obvious solution for a less capable programming language
given that no output at all was expected on stdout.

But the result is that the version number appears twice, and then gets misparsed
and ends up triggering bad assumptions in the code running cython.

It turns out that it's pretty easy to just print once, though. Detect when
stdout and stderr are redirected to the same location, and only print once.

See #5504
Fixes https://bugs.gentoo.org/911333
Dudemanguy pushed a commit to Dudemanguy/meson that referenced this issue Aug 7, 2023
@eli-schwartz @Dudemanguy
compilers: detect cython version on stdout for newer cython versions
ddf7bbc 
Cython historically, when asked to print the version and exit
successfully, would do so on stderr, which is weird and inconsistent.
Recently, it fixed this UX bug by printing on stdout instead:

cython/cython#5504

This then broke meson detection because we assumed it was on stderr due
to historically being there:

scipy/scipy#18865

Cython is right, and shouldn't have to revert this reasonable change for
backwards compatibility. Instead, check both.
@isc-adang isc-adang mentioned this issue Aug 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect Error Reporting
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

print --version to stdout [no ci] mattip/cython
4 participants
@scoder @mattip @eli-schwartz @da-woods