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

[chore]: Makefile.defs - use tabs everywhere and remove old unused code #752

Merged
merged 3 commits into from
Nov 10, 2018

Conversation

pazos
Copy link
Member

@pazos pazos commented Nov 8, 2018

Use tabs everywhere as talked in #745. Also I found old code which is not used anywhere. Remove it.

@pazos pazos force-pushed the tabs branch 3 times, most recently from 45c676c to d29c61a Compare November 8, 2018 19:44
Makefile.defs Outdated
COMPAT_CXXFLAGS:=$(UBUNTU_COMPAT_CFLAGS)
ifeq ($(shell PATH='$(PATH)' $(CC) -dumpmachine 2>/dev/null), arm-linux-gnueabi)
COMPAT_CFLAGS:=$(UBUNTU_COMPAT_CFLAGS)
COMPAT_CXXFLAGS:=$(UBUNTU_COMPAT_CFLAGS)
endif
Copy link
Member

Choose a reason for hiding this comment

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

whistles

$(info ************ Building for MACHINE: "$(MACHINE)" **********)
$(info ************ PATH: "$(PATH)" **********)
$(info ************ CHOST: "$(CHOST)" **********)
MACHINE:=$(MACHINE)-debug
Copy link
Member

Choose a reason for hiding this comment

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

I don't see it at a glance, but there must be a stray tab or something somewhere prior to here, unfortunately. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't understand this one :p

FYI: I'm checking in vim with

:syntax on
:set syntax=whitespace

and I see tabs there

Copy link
Member

Choose a reason for hiding this comment

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

L269 is the one with spaces, as @Frenzie pointed out ;).

Copy link
Member

Choose a reason for hiding this comment

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

Don't indent the lines with info and/or move them along these lines:

diff --git a/Makefile b/Makefile
index 8ab6d7a..3ccca4b 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,9 @@
 include Makefile.defs
 
+$(info ************ Building for MACHINE: "$(MACHINE)" **********)
+$(info ************ PATH: "$(PATH)" **********)
+$(info ************ CHOST: "$(CHOST)" **********)
+
 # main target
 all: $(OUTPUT_DIR)/libs $(if $(ANDROID),,$(LUAJIT)) \
                $(if $(USE_LUAJIT_LIB),$(LUAJIT_LIB),) \
diff --git a/Makefile.defs b/Makefile.defs
index b6f72ab..b88840b 100644
--- a/Makefile.defs
+++ b/Makefile.defs
@@ -460,9 +460,7 @@ endif
 MACHINE?=$(shell PATH='$(PATH)' $(CC) -dumpmachine 2>/dev/null)
 ifdef KODEBUG
        MACHINE:=$(MACHINE)-debug
-       $(info ************ Building for MACHINE: "$(MACHINE)" **********)
-       $(info ************ PATH: "$(PATH)" **********)
-       $(info ************ CHOST: "$(CHOST)" **********)
 endif
 OUTPUT_DIR?=build/$(MACHINE)

I'm inclined to think this little bit of crucial info shouldn't hide behind special debug flags anyway.

It might need to be done a touch more intelligently though.

@pazos
Copy link
Member Author

pazos commented Nov 10, 2018

Updated 👍

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Yeah, let's just go with this for now. Ideally we might want to only show it for target all or something since it's kinda weird info for android-toolchain and such. ;-)

@NiLuJe NiLuJe merged commit 89ce9cd into koreader:master Nov 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants