Build for factory UI JS/CSS. [chromiumos/third_party/autotest : master]

4 views
Skip to first unread message

Jon Salz (Code Review)

unread,
Apr 25, 2012, 2:15:05 PM4/25/12
to
Jon Salz has uploaded a new change for review.

Change subject: Build for factory UI JS/CSS.
......................................................................

Build for factory UI JS/CSS.

BUG=None
TEST=Built and run

Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
---
A client/build_deps/closure_library/.gitignore
A client/build_deps/closure_library/closure-library-20111110-r1376.tar.bz2
A client/cros/factory/static/.gitignore
A client/cros/factory/static/Makefile
4 files changed, 53 insertions(+), 0 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/autotest refs/changes/66/21066/1
--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Mike Frysinger (Code Review)

unread,
Apr 25, 2012, 2:22:46 PM4/25/12
to Jon Salz, Mike Frysinger
Mike Frysinger has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File client/cros/factory/static/Makefile
Line 12: CLOSURE_LIB_DIR := $(CLOSURE_LIB_DIR)$(CLOSURE_LIB_VERSION)
could just:

CLOSURE_LIB_DIR += $(CLOSURE_LIB_VERSION)

Line 40: tar xfj $(CLOSURE_LIB_ARCHIVE) -C $(dir $(CLOSURE_LIB_DIR))
if this only runs inside the chroot, you can drop the "j" flag -- tar will autodetect the file format and decompress as need be

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 25, 2012, 2:29:04 PM4/25/12
to Zdenek Behan, Chris Sosa, Mike Frysinger
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File client/cros/factory/static/Makefile
Line 12: CLOSURE_LIB_DIR := $(CLOSURE_LIB_DIR)$(CLOSURE_LIB_VERSION)
That will insert a space... I need this to be a line continuation.

(Or should I just suppress the 80-char limit here?)

Line 40: tar xfj $(CLOSURE_LIB_ARCHIVE) -C $(dir $(CLOSURE_LIB_DIR))
Done

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: Jon Salz <js...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Zdenek Behan <zbe...@chromium.org>

Mike Frysinger (Code Review)

unread,
Apr 25, 2012, 2:34:14 PM4/25/12
to Jon Salz, Zdenek Behan, Chris Sosa, Mike Frysinger
Mike Frysinger has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File client/cros/factory/static/Makefile
Line 12: CLOSURE_LIB_DIR := $(CLOSURE_LIB_DIR)$(CLOSURE_LIB_VERSION)
right, it'd have to be:

CLOSURE_LIB_DIR +=$(CLOSURE_LIB_VERSION)

doesn't matter to me either way. go with whichever you prefer.

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 25, 2012, 2:37:33 PM4/25/12
to Zdenek Behan, Chris Sosa, Mike Frysinger
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File client/cros/factory/static/Makefile
Line 12: CLOSURE_LIB_DIR := $(CLOSURE_LIB_DIR)$(CLOSURE_LIB_VERSION)
That adds a space too, I believe?

"""
A := X
A +=Y

$(error $(A))
"""

prints

"""
Makefile:5: *** X Y. Stop.
"""

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Mike Frysinger (Code Review)

unread,
Apr 25, 2012, 2:43:36 PM4/25/12
to Jon Salz, Zdenek Behan, Chris Sosa, Mike Frysinger
Mike Frysinger has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File client/cros/factory/static/Makefile
Line 12: CLOSURE_LIB_DIR := $(CLOSURE_LIB_DIR)$(CLOSURE_LIB_VERSION)
hrm, i didn't think it did, but your example says otherwise

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 26, 2012, 10:45:08 AM4/26/12
to Zdenek Behan, Chris Sosa, Mike Frysinger
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 2:

Chris, did you want to review this or should I just have someone from my team look?

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 26, 2012, 10:52:01 AM4/26/12
to Zdenek Behan, Chris Sosa, Mike Frysinger
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Chris Sosa (Code Review)

unread,
Apr 26, 2012, 4:23:13 PM4/26/12
to Jon Salz, Zdenek Behan, Mike Frysinger
Chris Sosa has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

Why is this being checked in as a tarball?

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Mike Frysinger (Code Review)

unread,
Apr 26, 2012, 4:25:05 PM4/26/12
to Jon Salz, Zdenek Behan, Chris Sosa, Mike Frysinger
Mike Frysinger has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

vs ... ? that isn't something we rolled (well, not exactly). see http://code.google.com/p/closure-library/downloads/list

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 26, 2012, 4:27:35 PM4/26/12
to Zdenek Behan, Chris Sosa, Mike Frysinger
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

I downloaded the .zip file and re-rolled it as a .tar.bz2 to save a few MB.

We could certainly commit it as naked files if you prefer! (But it's ~2000 files)

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Chris Sosa (Code Review)

unread,
Apr 26, 2012, 4:43:48 PM4/26/12
to Jon Salz, Zdenek Behan, Mike Frysinger
Chris Sosa has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

Nah what I was saying was that it seems the closure library itself should be pulled in as a sep package wrapped with an ebuild ... and this Makefile should just use the installed closure library to compile the factory ui code.

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 26, 2012, 4:47:18 PM4/26/12
to Zdenek Behan, Chris Sosa, Mike Frysinger
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

Ah I see! Yeah, that's what I originally thought to do, but Mike wrote:

'''
i don't think we have any ebuilds that install javascript libraries. their usage tends to be heavily web-install based, so packaging them up in the system doesn't model well (or at all?).

so your best bet would probably be to just include these in whatever is going to be actually using it.
'''

Of course I am OK either way.

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Mike Frysinger (Code Review)

unread,
Apr 26, 2012, 4:50:10 PM4/26/12
to Jon Salz, Zdenek Behan, Chris Sosa, Mike Frysinger
Mike Frysinger has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

yeah, what jon says i said ;)

in this case, my understanding is that it's the factory autotest logic that needs the compiled output of the closure library, so putting it here seems to make sense.

only other suggestion might be to have the autotest-factory ebuild itself pull down the .zip, but i'm not sure that's better. and wouldn't allow working directly in the autotest src tree.

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Chris Sosa (Code Review)

unread,
Apr 26, 2012, 4:57:22 PM4/26/12
to Jon Salz, Zdenek Behan, Mike Frysinger, Scott Zawalski
Chris Sosa has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

I think I'd prefer the latter suggestion of keeping this in the autotest-factory ebuild. Upstream autotest doesn't have any concepts of build_deps and since this doesn't really fit with client/deps keeping this outside would leave autotest cleaner.

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: Jon Salz <js...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Scott Zawalski <sco...@chromium.org>
Gerrit-Reviewer: Zdenek Behan <zbe...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 26, 2012, 5:08:44 PM4/26/12
to Zdenek Behan, Chris Sosa, Mike Frysinger, Scott Zawalski
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3:

Yeah I need to be able to work on it directly in the tree, so putting it in the ebuild would be a minor pain (although not the end of the world of course). Where would it download this from though (doesn't everything have to go through a mirror, which requires that the package is in Gentoo)?

The factory UI is in the process of getting moved out of autotest, so I would propose that if this isn't *too* terrible I just leave it as is for now. If it's still there in a week you can come over to my visitor cube in 1950 and kick my ass.

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Mike Frysinger (Code Review)

unread,
Apr 26, 2012, 6:00:15 PM4/26/12
to Jon Salz, Zdenek Behan, Chris Sosa, Mike Frysinger, Scott Zawalski
Mike Frysinger has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3: Looks good to me, but someone else must approve

i'm ok with lettings this slide until they migrate everything to factory_test_tools

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Chris Sosa (Code Review)

unread,
Apr 26, 2012, 6:18:34 PM4/26/12
to Jon Salz, Zdenek Behan, Mike Frysinger, Scott Zawalski
Chris Sosa has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3: Looks good to me, approved

Fair enough :)

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>

Jon Salz (Code Review)

unread,
Apr 26, 2012, 6:25:23 PM4/26/12
to Zdenek Behan, Chris Sosa, Mike Frysinger, Scott Zawalski
Jon Salz has posted comments on this change.

Change subject: Build for factory UI JS/CSS.
......................................................................


Patch Set 3: Ready

--
To view, visit https://gerrit.chromium.org/gerrit/21066
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I589d675766d8709abab9c0dc85ed0d221ed5038c
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Jon Salz <js...@chromium.org>
Reply all
Reply to author
Forward
0 new messages