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.

Sign up for GitHub

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

Jump to bottom

Fix Windows CI #36

Merged
merged 5 commits into from
Oct 17, 2017
Merged

Fix Windows CI #36

merged 5 commits into from
Oct 17, 2017

Conversation

alexeagle
Copy link
Collaborator

No description provided.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@alexeagle alexeagle changed the title Wip windows support Fix Windows CI Oct 6, 2017
alexeagle
alexeagle commented Oct 6, 2017
View reviewed changes
Copy link
Collaborator Author

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

I'll address these cleanups

.ci/rules_nodejs.json Outdated
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

revert

examples/rollup/BUILD.bazel Outdated
@@ -1,7 +1,8 @@
# NOTE: the example uses //:defs.bzl because it lives in the same WORKSPACE
# as the rule declarations.
# Users should use @build_bazel_rules_nodejs//:defs.bzl
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

update comment

internal/jasmine_runner.js Outdated
@@ -1,6 +1,6 @@
const fs = require('fs');
const path = require('path');
const JasmineRunner = require('jasmine');
const JasmineRunner = require('jasmine/lib/jasmine.js');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doesn't need .js

internal/jasmine_node_test.bzl Outdated
for p in [".", PACKAGE_NAME, "%s_devmode_srcs.MF" % name]
if p
])] + args
args = ["/".join([manifest.workspace_root.split("/")[1], manifest.package, manifest.name])] + args
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check if we still need this change

internal/node.bzl Outdated
substitutions={
"TEMPLATED_node": ctx.workspace_name + "/" + node.path,
"TEMPLATED_args": " ".join(ctx.attr.args),
"TEMPLATED_args": "",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need a todo here? restore the args?

internal/node.bzl Outdated
}

nodejs_binary = rule(
nodejs_binary_rule = rule(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

users will see this name in bazel query --output=label_kind so we should make this name match the name they use

internal/node.bzl Outdated
data = [":%s_loader" % name],
)

def nodejs_test(name, args=[], **kwargs):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment how it's duplicate of above intentionally

internal/node_launcher.sh
@@ -1,4 +1,4 @@
#!/usr/bin/env bash
#!/bin/bash
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is this change needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point I needed this change to run on windows, but we should double check if it's still needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confirmed, it's still needed.

internal/node_launcher.sh
if [[ -n "$TEST_SRCDIR" ]]; then
if [[ -n "$RUNFILES_MANIFEST_ONLY" ]]; then
# Windows only has a manifest file instead of symlinks.
RUNFILES=${RUNFILES_MANIFEST_FILE%/MANIFEST}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleanup: we trim the /MANIFEST only to add it back later

internal/node_loader.js Outdated
@@ -102,6 +101,7 @@ module.constructor._resolveFilename =
var resolveLocations = [
request,
resolveRunfiles(request),
resolveRunfiles('TEMPLATED_workspace_name', 'TEMPLATED_label_package', request),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

shouldn't need to add a fourth lookup location, at least without understanding why

Copy link
Contributor

Choose a reason for hiding this comment

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

I needed the fourth lookup to be able to import bundle.js in the rollup jasmine tests.

@alexeagle alexeagle mentioned this pull request Oct 13, 2017
Merged
@alexeagle alexeagle force-pushed the wip-windows-support branch from f94c41f to 112fa01 Compare October 16, 2017 17:47
@alexeagle alexeagle force-pushed the wip-windows-support branch 2 times, most recently from 5abc7db to 14f7f17 Compare October 16, 2017 20:57
@alexeagle alexeagle force-pushed the wip-windows-support branch 11 times, most recently from 2e241d8 to 2a9fac4 Compare October 17, 2017 03:14
@alexeagle alexeagle force-pushed the wip-windows-support branch from 2a9fac4 to a9c12ce Compare October 17, 2017 03:16
filipesilva
filipesilva approved these changes Oct 17, 2017
View reviewed changes
.ci/rules_nodejs.json Outdated
"configure": [
"%BAZEL% run @yarn//:yarn"
]
}
}
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove empty line.

internal/node_loader.js Outdated
@@ -107,7 +107,6 @@ module.constructor._resolveFilename =
var resolveLocations = [
request,
resolveRunfiles(request),
resolveRunfiles('TEMPLATED_workspace_name', 'TEMPLATED_label_package', request),
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it that removing this related to #32?

internal/jasmine_node_test.bzl Outdated
data += srcs + deps
data += [Label("//internal:jasmine_runner.js")]
data += [manifest]
data += [":%s_devmode_srcs.MF" % name]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alexeagle
Copy link
Collaborator Author

FYI @dslomov :)

@alexeagle alexeagle merged commit 503da45 into master Oct 17, 2017
@alexeagle alexeagle deleted the wip-windows-support branch October 17, 2017 12:17
@alexeagle alexeagle mentioned this pull request Oct 17, 2017
Closed
@dslomov
Copy link
Contributor

dslomov commented Oct 18, 2017

Cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filipesilva filipesilva filipesilva approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexeagle @googlebot @dslomov @filipesilva