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

Introduce webfiles() and development web server #153

Closed
wants to merge 1 commit into from

Conversation

jart
Copy link
Contributor

@jart jart commented Oct 10, 2016

Reviewer: @danmane

This webfiles() rule has been designed primarily for supporting Polymer web components, but it's general purpose enough to serve the needs of any web developer. It allows each piece of a website to be defined in terms of the relationships between source files. This is validated by checking the src/href/url attributes to make sure the referenced files exist. Vulcanization support will also be reinvented shortly.

Each webfiles() rule is executable. It starts a lightweight Jetty web server that serves up the transitive closure of dependencies with directory listings and live source reloading.

This rule is capable of serving as a Fileset() replacement, which means it will be extended in the near future create zip/war archives.

Copy link

@teamdandelion teamdandelion left a comment

Choose a reason for hiding this comment

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

Seems great, but I am not very familiar with skylark/java/closure rules so I'm not sure I would have caught any Really Bad Things.

manifest_lines = [ctx.label]
for src in ctx.attr.srcs:
sname = "/" + src.label.name
# TODO(jart): Audit algorithm for subpath extraction.

Choose a reason for hiding this comment

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

Should this TODO make it into the codebase?

for dep in ctx.attr.deps:
inputs.append(dep.webfiles.dummy)
for f in dep.files:
inputs.append(f)

Choose a reason for hiding this comment

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

Equivalent to inputs.extend(dep.files), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no.

ERROR: /usr/local/google/home/jart/.cache/bazel/_bazel_jart/9c19469f1b82ef6dc9e1d1c576c7b609/external/iron_flex_layout/BUILD:9:1: in webfiles rule @iron_flex_layout//:iron_flex_layout: 
Traceback (most recent call last):
        File "/usr/local/google/home/jart/.cache/bazel/_bazel_jart/9c19469f1b82ef6dc9e1d1c576c7b609/external/iron_flex_layout/BUILD", line 9
                webfiles(name = 'iron_flex_layout')
        File "/usr/local/google/home/jart/.cache/bazel/_bazel_jart/9c19469f1b82ef6dc9e1d1c576c7b609/external/io_bazel_rules_closure/closure/webfiles.bzl", line 69, in _webfiles
                inputs.extend(dep.files)
Method list.extend(items: sequence) is not applicable for arguments (set): 'items' is set, but should be sequence.
ERROR: Analysis of target '//tensorflow/tensorboard/components/vz-line-chart/demo:demo' failed; build aborted.

.put("zip", MediaType.ZIP)
.build();

static final ImmutableSet<MediaType> COMPRESSABLE =

Choose a reason for hiding this comment

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

s/COMPRESSABLE/COMPRESSIBLE/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

private static final String BOLD = "\u001b[1m";
private static final String RESET = "\u001b[0m";

private static final HostAndPort DEFAULT_BIND = HostAndPort.fromString("[::]:6006");

Choose a reason for hiding this comment

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

This will conflict with the default on TensorBoard, which I think is slightly inconvenient.
6006 was chosen for GOOG.
Maybe this time the default should be 600613?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

600613 > 2**16-1 so no dice. But this server has already got you covered. If it can't bind to 6006, it increments the port number and tries again.

}
}
Server server;
while (true) {

Choose a reason for hiding this comment

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

The while(true) is so it will keep incrementing ports until it gets one that works?
Maybe comment it or factor it out (if feasible)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

verify(!path.isAbsolute());
path = RUNFILES_PATH.resolve(path);
if (path == null) {
serveError(SC_BAD_REQUEST, "Naughty naughty!");

Choose a reason for hiding this comment

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

Maybe something a bit more descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually come to think of it, this isn't needed.

@jart
Copy link
Contributor Author

jart commented Oct 12, 2016

PTAL

@jart jart force-pushed the webfiles branch 2 times, most recently from 5a67415 to b1879fb Compare October 12, 2016 22:37
@jart
Copy link
Contributor Author

jart commented Oct 13, 2016

By the way, I haven't really written a lot of unit tests for this change, even though I should. I did this because I'm favoring velocity. But the tests would make it clearer what the code is doing and prove that it works. So if you want me to write them now rather than later, I will completely understand.

The webfiles() rule has been designed primarily for supporting Polymer
web components, but it's general purpose enough to serve the needs of
any web developer. It allows each piece of a website to be defined in
terms of the relationships between source files. This is validated by
checking the src/href/url attributes to make sure the referenced files
exist.  Vulcanization support will also be reinvented shortly.

Each webfiles() rule is executable. It starts a lightweight Jetty web
server that serves up the transitive closure of dependencies with
directory listings and live source reloading.

This rule is capable of serving as a Fileset() replacement, which means
it will be extended in the near future create zip/war archives.
@jmhodges
Copy link

(This test failure just seems to have been a disk space problem on one machine if the review is waiting for it to go green)

@jart
Copy link
Contributor Author

jart commented Nov 11, 2016

Closing this PR because it's patched as cl/138836345

@jart jart closed this Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants