Issue 2082015: add cgpt framework and attribute support.

Issue 2082015: add cgpt framework and attribute support. (Closed)

Created:
10 years, 7 months ago by Louis

Modified:
9 years, 4 months ago

Reviewers:
Bill Richardson

CC:
chromium-os-reviews_chromium.org

Base URL:
ssh://git@chromiumos-git/chromeos

Visibility:
Public.

Description

add cgpt framework and attribute support.

Patch Set 1 #

Total comments: 12

Created: 10 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+991 lines, -79 lines)			Patch
M	src/platform/vboot_reference/Makefile	View	1 chunk	+5 lines, -3 lines	4 comments	Download
M	src/platform/vboot_reference/cgptlib/Makefile	View	1 chunk	+6 lines, -4 lines	4 comments	Download
M	src/platform/vboot_reference/cgptlib/cgptlib.h	View	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/platform/vboot_reference/cgptlib/cgptlib.c	View	2 chunks	+29 lines, -7 lines	0 comments	Download
M	src/platform/vboot_reference/cgptlib/cgptlib_internal.h	View	3 chunks	+10 lines, -6 lines	0 comments	Download
M	src/platform/vboot_reference/cgptlib/crc32.c	View	1 chunk	+54 lines, -54 lines	0 comments	Download
M	src/platform/vboot_reference/cgptlib/tests/Makefile	View	1 chunk	+4 lines, -3 lines	0 comments	Download
M	src/platform/vboot_reference/cgptlib/tests/cgptlib_test.c	View	2 chunks	+2 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/cgptlib/tests/crc32_test.h	View	1 chunk	+10 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/cgptlib/tests/crc32_test.c	View	1 chunk	+47 lines, -0 lines	0 comments	Download
M	src/platform/vboot_reference/utility/Makefile	View	3 chunks	+13 lines, -2 lines	4 comments	Download
A	src/platform/vboot_reference/utility/cgpt/Makefile	View	1 chunk	+20 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/utility/cgpt/cgpt.h	View	1 chunk	+121 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/utility/cgpt/cgpt.c	View	1 chunk	+246 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/utility/cgpt/cgpt_attribute.c	View	1 chunk	+147 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/utility/cgpt/cgpt_options.c	View	1 chunk	+122 lines, -0 lines	0 comments	Download
A	src/platform/vboot_reference/utility/cgpt/cgpt_show.c	View	1 chunk	+153 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Bill Richardson

LGTM, but try my suggested Makefile changes first. http://codereview.chromium.org/2082015/diff/1/2 File src/platform/vboot_reference/Makefile (right): http://codereview.chromium.org/2082015/diff/1/2#newcode17 src/platform/vboot_reference/Makefile:17: set ...

10 years, 7 months ago (2010-05-20 16:51:15 UTC) #2

Louis

Waw! Your parallelization idea is really cool. But unfortunately, we need to build targets in ...

10 years, 7 months ago (2010-05-21 03:43:21 UTC) #3

Waw! Your parallelization idea is really cool. But unfortunately, we need to
build targets in order (common->lib*->binaries), but 'make -j' always makes it
out-of-order.

Our build system actually uses "make clean all" and accidentally triggers 'make
all' in '%-subdir:'. So that I cannot use parallelization in 'clean' target.

But I will definitely notice parallelization in future. Will use it if possible.

http://codereview.chromium.org/2082015/diff/1/2
File src/platform/vboot_reference/Makefile (right):

http://codereview.chromium.org/2082015/diff/1/2#newcode17
src/platform/vboot_reference/Makefile:17: set -e; \
On 2010/05/20 16:51:15, Bill Richardson wrote:
> That's not parallelizable. Use this instead:
> 
> all: $(foreach DIR,$(SUBDIRS),$(DIR)-subdir)
> 
> %-subdir :
>         $(MAKE) -C $(subst -subdir,,$@) $(MAKECMDGOALS)
> 

This cannot be parallelized because we need to compile common first, then *lib,
then rest binaries.

http://codereview.chromium.org/2082015/diff/1/2#newcode23
src/platform/vboot_reference/Makefile:23: set -e; \
On 2010/05/20 16:51:15, Bill Richardson wrote:
> clean: $(foreach DIR,$(SUBDIRS),$(DIR)-subdir) 

This cannot be parallelized because our build system uses "make clean all" and
accidentally triggers 'make all' in '%-subdir:'.

http://codereview.chromium.org/2082015/diff/1/3
File src/platform/vboot_reference/cgptlib/Makefile (right):

http://codereview.chromium.org/2082015/diff/1/3#newcode12
src/platform/vboot_reference/cgptlib/Makefile:12: $(MAKE) -C $$i; \
On 2010/05/20 16:51:15, Bill Richardson wrote:
> That's not parallelizable. Use this instead:
> 
> all: libcgpt.a $(foreach DIR,$(SUBDIRS),$(DIR)-subdir)
> 
> %-subdir :
>         $(MAKE) -C $(subst -subdir,,$@) $(MAKECMDGOALS)
> 

This cannot be parallelized because we need libcgpt.a be ready before compiling
subdirs.

http://codereview.chromium.org/2082015/diff/1/3#newcode23
src/platform/vboot_reference/cgptlib/Makefile:23: for i in $(SUBDIRS); do \
On 2010/05/20 16:51:15, Bill Richardson wrote:
> clean: $(foreach DIR,$(SUBDIRS),$(DIR)-subdir) 

This cannot be parallelized because our build system uses "make clean all" and
accidentally triggers 'make all' in '%-subdir:'.

http://codereview.chromium.org/2082015/diff/1/12
File src/platform/vboot_reference/utility/Makefile (right):

http://codereview.chromium.org/2082015/diff/1/12#newcode33
src/platform/vboot_reference/utility/Makefile:33: subdirs:
On 2010/05/20 16:51:15, Bill Richardson wrote:
> That's not parallelizable. Use this instead: 
> 
> subdirs: $(foreach DIR,$(SUBDIRS),$(DIR)-subdir)
> 
> %-subdir :
>         $(MAKE) -C $(subst -subdir,,$@) $(MAKECMDGOALS)
> 
> 

Done.

http://codereview.chromium.org/2082015/diff/1/12#newcode60
src/platform/vboot_reference/utility/Makefile:60: for i in $(SUBDIRS); do \
On 2010/05/20 16:51:15, Bill Richardson wrote:
> clean: $(foreach DIR,$(SUBDIRS),$(DIR)-subdir)
> 

This cannot be parallelized because our build system uses "make clean all" and
accidentally triggers 'make all' in '%-subdir:'.

Okay, then. LGTM.

Expand Messages | Collapse Messages