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

sunxi-tools 1.4 make fails when building meminfo #70

Closed
daym opened this issue Oct 26, 2016 · 10 comments
Closed

sunxi-tools 1.4 make fails when building meminfo #70

daym opened this issue Oct 26, 2016 · 10 comments

Comments

@daym
Copy link
Contributor

daym commented Oct 26, 2016

$ make TARGET_TOOLS=meminfo
...cc -g -O0 -Wall -Wextra  -std=c99 -D_POSIX_C_SOURCE=200112L -D_BSD_SOURCE -D_DEFAULT_SOURCE -Iinclude/    meminfo.c Makefile common.h version.h   -o meminfo
....Makefile: file not recognized: File format not recognized

That is because we add Makefile, common.h and version.h as dependencies to all the tools (which I think is good).

However, the entry for sunxi-meminfo says:

sunxi-meminfo: meminfo.c
        $(CROSS_COMPILE)gcc -g -O0 -Wall -static -o $@ $^

And $^ will pass the Makefile to the compiler - which will barf.

I'd pass $<

Same for sunxi-script_extractor and sunxi-fel and the generic sunxi-% rule.

@daym daym changed the title sunxi-tools 1.4 Makefile fails when building meminfo sunxi-tools 1.4 make fails when building meminfo Oct 26, 2016
@daym
Copy link
Contributor Author

daym commented Oct 26, 2016

See pull request #71

@n1tehawk
Copy link
Collaborator

Oh... I've never thought of invoking make that way, overriding the TARGET_TOOLS variable. The original intent was that users would use make sunxi-meminfo, possibly with a custom CROSS_COMPILE setting, e.g.

sunxi-tools # make -B sunxi-meminfo CROSS_COMPILE=armv7a-hardfloat-linux-gnueabi-
armv7a-hardfloat-linux-gnueabi-gcc -g -O0 -Wall -static -o sunxi-meminfo meminfo.c

This is also what README.md states.

Regards, NiteHawk

@daym
Copy link
Contributor Author

daym commented Oct 26, 2016

Well yeah, but that's what TARGET_TOOLS is supposed to be for, right: To list tools which only work on the target...

@n1tehawk
Copy link
Collaborator

You're right - it took me a while to see all the implications. For those targets that have a single .c file dependency, the proposed change from $^ to $< is a good suggestion.

However the longer I look at it, the more I get the impression that we're lacking a dedicated rule for sunxi-pio, meaning it gets built in the wrong way - namely using the native compiler of the host system instead of the cross-compiler aiming at the target.

Fixing that (by adding a proper rule) will also mean that me might "suddenly" see build failures for systems where no cross-compiler is available, because so far the target-tools is part of the default make target. I wonder if it would be preferable to make tools the default, and require users to explicitly specify make all if they also want the target-tools...

Regards, NiteHawk

@n1tehawk
Copy link
Collaborator

Hi Danny!

I'd like your feedback on this branch, especially the commit that alters the Makefile behaviour, related to the "breakage" concerns I expressed above. Would you be okay with solving it that way?

Regards, NiteHawk

@daym
Copy link
Contributor Author

daym commented Oct 27, 2016

Hi NiteHawk,

I've tested the branch and it works.

I see that it now leaves off useless-on-that-platform programs by default. Also, you added meminfo which is nice :)

(I've had to help someone with memory stability problems (read: complete crashes) on his lime2. Also, apt-get install sunxi-tools did not install sunxi-meminfo. That was a really bad user experience)

n1tehawk added a commit to n1tehawk/sunxi-tools that referenced this issue Oct 28, 2016
This appends sunxi-meminfo to the TARGET_TOOLS, and adds a new rule
to fix the compilation of sunxi-pio (by making it *cross-compile*
for the target).

Additionally adds a new build target "make install-misc".

For more details, see github issues linux-sunxi#69 and linux-sunxi#70.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
n1tehawk added a commit to n1tehawk/sunxi-tools that referenced this issue Oct 28, 2016
This appends sunxi-meminfo to the TARGET_TOOLS, and adds a new rule
to fix the compilation of sunxi-pio (by making it *cross-compile*
for the target).

Additionally adds a new build target "make install-misc".

For more details, see github issues linux-sunxi#69 and linux-sunxi#70.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
@daym
Copy link
Contributor Author

daym commented Oct 29, 2016

Thinking about it some more, gcc ... $(filter %.c,$^) does the right thing - passing just the C files to gcc. That way one could conceivably add c files to common targets like $(TOOLS) and they would be picked up.

I realize that I was the one that suggested $< - I changed my mind :) .

I think $(filter %.c,$^) is better. (But don't use only $^)

@n1tehawk
Copy link
Collaborator

I'm fine with using $< for the target-tools, where we have explicit rules depending on a single .c file anyway. Should the need arise, we could still move those to $filter() individually.

And yes, $^ backfired in a few cases after the "global" dependencies were extended - which did not get caught by our CI tests. I've tweaked the Travis configuration a bit to attempt building and installing all binaries, hopefully covering more corner cases in the future.

n1tehawk added a commit to n1tehawk/sunxi-tools that referenced this issue Oct 29, 2016
This appends sunxi-meminfo to the TARGET_TOOLS, and adds a new rule
to fix the compilation of sunxi-pio (by making it *cross-compile*
for the target).

Additionally adds a new build target "make install-misc".

For more details, see github issues linux-sunxi#69 and linux-sunxi#70.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
@n1tehawk
Copy link
Collaborator

n1tehawk commented Oct 29, 2016

Changes were merged. v1.4.1 should have resolved this issue.

Regards, NiteHawk

@n1tehawk
Copy link
Collaborator

n1tehawk commented Nov 6, 2016

Unfortunately, the sunxi-pio change introduced as part of #73 is wrong. 6cd51fe fixes it.

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

No branches or pull requests

2 participants