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

Prettify makefile output. #254

Merged
1 commit merged into from
Apr 9, 2018
Merged

Prettify makefile output. #254

1 commit merged into from
Apr 9, 2018

Conversation

Szunti
Copy link
Contributor

@Szunti Szunti commented Mar 29, 2018

Writes no/yes instead of empty/yes. Fixes #194.

@@ -27,27 +27,28 @@ OPTIMIZE?=yes
USE_LUA?=yes
USE_BOX2D?=yes
CCACHE?=ccache
USE_CCACHE?=$(shell which $(CCACHE) 2>&1 > /dev/null && echo yes)
USE_CCACHE?=$(shell which $(CCACHE) > /dev/null 2>&1 && echo yes)
Copy link

@ghost ghost Mar 29, 2018

Choose a reason for hiding this comment

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

To my understanding, by issuing:

USE_CCACHE=yes make

$(CCACHE) detection would get wrongly passed by, as an undesired side effect of making explicit an otherwise implicit value.

So to be perfect, I would consider separating the default value setting (USE_CCACHE?=yes) from $(CCACHE) detection, this latest then done using an inconditional assignment operator = (USE_CCACHE=$(shell which $(CCACHE) >/dev/null 2>&1 && echo yes) only under ifeq ($(USE_CCACHE), yes).

Copy link

Choose a reason for hiding this comment

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

In case you didn't knew. If the intention is to silence output completely, the ice cream operator does it simpler, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that bash has it and I use it, but I know nothing about other shells.

Copy link

Choose a reason for hiding this comment

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

Hmmm. Smart. Insightful.

$ bash
$ which sh
/bin/sh
$ file /bin/sh
/bin/sh: symbolic link to dash
$ echo '#include <stdio.h>' >test.c
$ echo 'main(){fprintf(stdout,"stdout\n");fprintf(stderr,"stderr\n");}' >>test.c
$ gcc -Wno-implicit-int test.c -o test
$ bash -c './test &>doc0'
$ dash -c './test &>doc1'
stdout
stderr
$ more test.c doc0 doc1
::::::::::::::
test.c
::::::::::::::
#include <stdio.h>
main(){fprintf(stdout,"stdout\n");fprintf(stderr,"stderr\n");}
::::::::::::::
doc0
::::::::::::::
stderr
stdout
::::::::::::::
doc1
::::::::::::::
$ bash -c './test >doc2 2>&1'
$ dash -c './test >doc3 2>&1'
$ more doc2 doc3
::::::::::::::
doc2
::::::::::::::
stderr
stdout
::::::::::::::
doc3
::::::::::::::
stderr
stdout
$ _

Very right. Thanks!

@ghost
Copy link

ghost commented Apr 9, 2018

If you open pull request from the default branch of your GitHub fork bad stuff always happens.

Best case scenario is you risk your pull request to complicate unnecessarily with merge commits, worst case scenario is you can see yourself pressured into freezing the default branch of your fork until the pull request resolves.

Keep using GitHub flow like you did at github/davewx7/citadel/pull/203. I see you created the feature branch here, but somehow the pull request isn't open from it but from the default branch.

@ghost ghost merged commit bd77a29 into anura-engine:trunk Apr 9, 2018
@Szunti
Copy link
Contributor Author

Szunti commented Apr 10, 2018

I made another branch in my repo and looked for hours if there is a way to change the branch a bit after opening the pull request, found none without opening a new pull request. But then your comments on the code would have been in a different PR.

I haven't added the changes regarding

USE_CCACHE=yes make

you mentioned yet.

@ghost
Copy link

ghost commented Apr 10, 2018

Regarding USE_CCACHE=yes make, would a very rare use, and unrelated to #194, so there would be no point in blocking this PR because of that. Can happen though:

$ su -c 'aptitude purge ccache' && make clean && USE_CCACHE=yes make
Password:
The following packages will be REMOVED:  
  ccache{p}
0 packages upgraded, 0 newly installed, 1 to remove and 85 not upgraded.
Need to get 0 B of archives. After unpacking 311 kB will be freed.
Do you want to continue? [Y/n/?] Y
(Reading database ... 291685 files and directories currently installed.)
Removing ccache (3.3.4-1) ...
Processing triggers for man-db (2.7.6.1-2) ...
                                         
rm -f build/kre/*.o build/svg/*.o build/Box2D/*.o build/tiled/*.o build/hex/*.o 
build/xhtml/*.o build/eris/*.o build/*.o build/kre/*.o.d build/svg/*.o.d build/B
ox2D/*.o.d build/tiled/*.o.d build/hex/*.o.d build/xhtml/*.o.d build/eris/*.o.d 
build/*.o.d anura
-e  OPTIMIZE            : yes
 USE_CCACHE          : yes
 CCACHE              : ccache
 SANITIZE_ADDRESS    : no
 SANITIZE_UNDEFINED  : no
 USE_DB_CLIENT       : no
 USE_BOX2D           : yes
 USE_LIBVPX          : no
 USE_LUA             : yes
 USE_SDL2            : yes
 CXX                 : g++
 BASE_CXXFLAGS       : -O2 -Wall -Werror -Wno-literal-suffix -Wno-sign-compare -
Wsuggest-override -fdiagnostics-color=auto -I/usr/include/SDL2 -D_REENTRANT -DUS
E_LUA -std=c++0x -g -fno-inline-functions -fthreadsafe-statics -Wno-narrowing -W
no-reorder -Wno-unused -Wno-unknown-pragmas -Wno-overloaded-virtual -DUSE_SVG -D
USE_IMGUI
 CXXFLAGS            :
 LDFLAGS             : -rdynamic
 INC                 : -isystem external/header-only-libs -D_REENTRANT -I/usr/in
clude/libdrm -I/usr/include/SDL2 -I/usr/include/cairo -I/usr/include/glib-2.0 -I
/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/includ
e/freetype2 -I/usr/include/libpng16 -I/usr/include/cairo -I/usr/include/glib-2.0
 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1-I/usr/incl
ude/freetype2 -I/usr/include/libpng16 -Iimgui
 LIBS                : -lX11 -lGL -D_REENTRANT -I/usr/include/libdrm -I/usr/incl
ude/SDL2 -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gn
u/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/inclu
de/libpng16 -lX11 -lGLEW -lGLU -lGL -lSDL2_image -lSDL2_ttf -lSDL2 -lpng16 -lz -
lfreetype -lcairo -logg -lvorbis -lvorbisfile -lrt -lcairo
Building: src/kre/SceneTree.cpp
make: ccache: Command not found
Makefile:234: recipe for target 'build/kre/SceneTree.o' failed
make: *** [build/kre/SceneTree.o] Error 127
$ _

This pull request was closed.
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.

1 participant