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

FS#2086 - IS_TTY in the makefile is broken #6943

Closed
openwrt-bot opened this issue Jan 27, 2019 · 4 comments
Closed

FS#2086 - IS_TTY in the makefile is broken #6943

openwrt-bot opened this issue Jan 27, 2019 · 4 comments
Labels

Comments

@openwrt-bot
Copy link

rdiez:

File include/toplevel.mk tries to find out whether the build is running in an interactive console like this:

export IS_TTY=$(shell tty -s && echo 1 || echo 0)

That is the wrong way to do it. The code above is testing stdin, and not stdout. As a result, piping to a log file with a tool like 'tee' yields a log file that still contains the same \r codes etc. as when running on an interactive console.

Other tools like systemd's systemctl do it right by checking stdout. On an interactive console, run these 2 commands to see the difference:

systemctl --help
systemctl --help | tee test.txt

It turns out that checking stdout is not so easy to do inside a makefile. In the shell, you would normally check with "-t 1", but a $(shell ...) construct inside a makefile won't work, because $(shell) always redirects stdout in order to capture the command's output.

See here for the gory details:

https://stackoverflow.com/questions/44097324/timing-of-makefile-include-statements-for-auto-generated-files/44133082

While there are workarounds for makefiles, I wouldn't go that far. I would just stop trying. and let the user decide with some variable. Maybe some day there will be a wrapper shell script, instead of using the makefile directly. In such a wrapper script it would be trivial to check for things like interactive terminal and/or terminal color support.

@openwrt-bot
Copy link
Author

rdiez:

I recently learnt that GNU Make has variables for this purpose:

New variables: $(MAKE_TERMOUT) and $(MAKE_TERMERR) are set to non-empty
values if stdout or stderr, respectively, are believed to be writing to a
terminal. These variables are exported by default.

I have attached a patch to fix this issue in OpenWrt's build system. Please treat the patch as a suggestion. If something is not right with it, please fix this issue in whichever way you find suitable.

@openwrt-bot
Copy link
Author

rdiez:

I have jumped through all the hoops and posted the patch with Git, which was automatically picked up by Patchwork. It is here:

build: IS_TTY is now set according to GNU Make's MAKE_TERMOUT

https://patchwork.ozlabs.org/project/openwrt/patch/mailman.21997.1591781638.2542.openwrt-devel@lists.openwrt.org/

The reason why the patch was rejected is:

Currently OpenWrt requires "GNU make v3.82", see include/prereq-build.mk

http://lists.infradead.org/pipermail/openwrt-devel/2020-June/029772.html

However, there is no other practical way to fix this issue, as far as I know.

GNU Make v3.82 is 10 years old, and the one we would need, version 4.1, is 6 years old.

I could try to change the Makefile logic to identify the GNU Make version, but the way OpenWrt makes me jump through hoops to post patches has discouraged me.

Given that the IS_TTY logic is currently broken, I would still apply the patch. If an older version of GNU Make without MAKE_TERMOUT runs this line:

export IS_TTY=$(if $(MAKE_TERMOUT),1,0)

Such an old versoin will then always decide that the output is not a TTY, and you will lose message colours on your terminal. That is not a big deal. The flip side is that your build log files will no longer have terminal rubbish inside.

@openwrt-bot
Copy link
Author

aparcar0:

This patch is required for this to work:
https://patchwork.ozlabs.org/project/openwrt/patch/20201213071702.154612-1-mail@aparcar.org/

@KanjiMonster
Copy link
Member

The fix was applied in a015d91.

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

No branches or pull requests

2 participants