Hi Julius,
On 2 October 2015 at 23:18, Julius Werner jwerner@chromium.org wrote:
I get this build error with sandbox.
test/built-in.o: In function `uncompress_using_lz4': /home/sjg/c/src/third_party/u-boot/files/test/compression.c:278: undefined reference to `ulz4fn'
Yeah... that's because you didn't configure it in. I'm not really sure how this is supposed to work... there should really be some sort of dependency between selecting the compression tests and selecting the algorithm, or the algorithms should be #ifdefed out like in bootm.c. But neither of those seems to be in place right now... include/configs/sandbox.h just enables all the other compression algorithms, that's why it works for them.
I could add a 'select LZ4' to config UNIT_TEST in the Kconfig to create such a dependency, I think that would be the best option?
Sounds good. I think it makes sense to enable all possible options in sandbox - since it helps with build testing too.
You should be able to run the tests using:
make O=sandbox defconfig all ./sandbox/u-boot -c "ut_compression"
I tried, but I can't find myself through the SDL dependency hell... sorry. I installed Ubuntu's libsdl2-dev but apparently that wasn't enough... I still get "make[2]: sdl-config: Command not found" and "../arch/sandbox/cpu/sdl.c:9:21: fatal error: SDL/SDL.h: No such file or directory" when I'm trying to build. (I ran it with make -k now so I confirmed that it builds both lib/lz4_wrapper.o and test/compression.o without errors. I just can't test the linking step.)
You can build U-Boot with NO_SDL=1
I have this on my goobuntu laptop:
~> dpkg -l |grep -i sdl ii libsdl1.2-dev 1.2.15-8ubuntu1.1 amd64 Simple DirectMedia Layer development files ii libsdl1.2debian:amd64 1.2.15-8ubuntu1.1 amd64 Simple DirectMedia Layer
Can you instead add this option to lib/Kconfig and put your help there? We are moving away from the old CONFIGS.
Done in next version.
You should be able to replace this license with
SPDX-License-Identifier: BSD-2-Clause [...] Should be able to use:
SPDX-License-Identifier: GPL-2.0+ BSD-2-Clause
Done in next version.
I can see why you want to keep lz4.c as is. But this file is written by you, isn't it? If so, can you fix the checkpatch errors that are fixable (e.g. run 'patman').
warning: lib/lz4_wrapper.c,41: do not add new typedefs warning: lib/lz4_wrapper.c,42: do not add new typedefs warning: lib/lz4_wrapper.c,43: do not add new typedefs warning: lib/lz4_wrapper.c,44: do not add new typedefs warning: lib/lz4_wrapper.c,45: do not add new typedefs warning: lib/lz4_wrapper.c,47: storage class should be at the beginning of the declaration error: lib/lz4_wrapper.c,59: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,60: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,61: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,62: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,63: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,64: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,70: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,71: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,72: spaces prohibited around that ':' (ctx:WxW) warning: lib/lz4_wrapper.c,77: __packed is preferred over __attribute__((packed)) warning: lib/lz4_wrapper.c,77: Adding new packed members is to be done with care error: lib/lz4_wrapper.c,83: spaces prohibited around that ':' (ctx:WxW) error: lib/lz4_wrapper.c,84: spaces prohibited around that ':' (ctx:WxW) warning: lib/lz4_wrapper.c,89: __packed is preferred over __attribute__((packed)) warning: lib/lz4_wrapper.c,89: Adding new packed members is to be done with care error: test/compression.c,282: return is not a function, parentheses are not required
Okay, I replaced __attribute__((packed)) with __packed and fixed the whitespace for bit fields. I think those are the only actionable ones... the camel case comes from names inside lz4.c, and I need the typedefs to map U-Boot's types to the ones lz4.c expects.
Right.
Could you return int here, and use proper error numbers below? Like -EINVAL, -EPROTONOSUPPORT, -ENOSPC, etc.
Okay, I switched it to the model where it returns an int and the size is an output pointer, like the other algorithms.
Sounds good.
Regards, Simon