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

rules_closure job on ci.bazel.io is broken @HEAD #32

Closed
damienmg opened this issue Apr 1, 2016 · 11 comments
Closed

rules_closure job on ci.bazel.io is broken @HEAD #32

damienmg opened this issue Apr 1, 2016 · 11 comments
Assignees
Labels

Comments

@damienmg
Copy link
Contributor

damienmg commented Apr 1, 2016

See http://ci.bazel.io/job/rules_closure/BAZEL_VERSION=HEAD,PLATFORM_NAME=linux-x86_64/26/console

Error:

____[84 / 162] Creating source manifest for //closure/testing/test:arithmetic_scope_test
ERROR: /home/ci/workspace/rules_closure/BAZEL_VERSION/HEAD/PLATFORM_NAME/linux-x86_64/closure/compiler/test/BUILD:106:1: Sanity checking 1 JS files in //closure/compiler/test:es6arrow_lib failed: namespace-sandbox failed: error executing command 
  (cd /home/ci/.cache/bazel/_bazel_ci/f4ef80a8a473f3eedd2e46be717300bf/linux-x86_64 && \
  exec env - \
  /home/ci/.cache/bazel/_bazel_ci/f4ef80a8a473f3eedd2e46be717300bf/linux-x86_64/_bin/namespace-sandbox @/home/ci/.cache/bazel/_bazel_ci/f4ef80a8a473f3eedd2e46be717300bf/linux-x86_64/bazel-sandbox/a8b6fde1-b159-417f-8260-9e552f6a4725-0.params -- bazel-out/local-fastbuild/bin/closure/compiler/jschecker/jschecker '--output_file=bazel-out/local-fastbuild/bin/closure/compiler/test/es6arrow_lib-provided.txt' '--label=//closure/compiler/test:es6arrow_lib' '--src=closure/compiler/test/es6arrow.js').
Traceback (most recent call last):
  File "/home/ci/.cache/bazel/_bazel_ci/f4ef80a8a473f3eedd2e46be717300bf/linux-x86_64/bazel-out/local-fastbuild/bin/closure/compiler/jschecker/jschecker.runfiles/io_bazel_rules_closure/closure/compiler/jschecker/jschecker.py", line 47, in <module>
    from external.closure_library.closure.bin.build import source
ImportError: No module named external.closure_library.closure.bin.build
____Building complete.
____Elapsed time: 7.129s, Critical Path: 2.67s

My opinion: the causing import statement should just be from closure.bin.build import source and it should work

@jart
Copy link
Contributor

jart commented Apr 1, 2016

Are you sure your fix works for Python? I get the following error:

upgrade-bazel jart@compy:~/code/rules_closure$ bazel run //closure/compiler/jschecker
INFO: Found 1 target...
Target //closure/compiler/jschecker:jschecker up-to-date:
  bazel-bin/closure/compiler/jschecker/jschecker
INFO: Elapsed time: 0.103s, Critical Path: 0.00s

INFO: Running command line: bazel-bin/closure/compiler/jschecker/jschecker
Traceback (most recent call last):
  File "/usr/local/google/home/jart/.cache/bazel/_bazel_jart/b80c5b3286c838833aa898c3f736cbae/rules_closure/bazel-out/local-fastbuild/bin/closure/compiler/jschecker/jschecker.runfiles/io_bazel_rules_closure/closure/compiler/jschecker/jschecker.py", line 47, in <module>
    from closure.bin.build import source
ImportError: No module named closure.bin.build
ERROR: Non-zero return code '1' from command: Process exited with status 1.

It appears that the generated Python runner script does:

  external_dir = os.path.join(module_space, 'external')
  print external_dir
  if os.path.isdir(external_dir):
    external_entries = [os.path.join(external_dir, d) for d in os.listdir(external_dir)]
    repositories = [d for d in external_entries if os.path.isdir(d)]
    print repositories
    python_path_entries += repositories

But the generated structure is now:

./bazel-bin/closure/compiler/jschecker/jschecker.runfiles
├── io_bazel_rules_closure/
│   ├── closure/
│   │   ├── compiler/
│   │   │   ├── __init__.py*
│   │   │   └── jschecker/
│   │   │       ├── __init__.py*
│   │   │       ├── jschecker
│   │   │       └── jschecker.py
│   │   └── __init__.py*
│   └── external/
│       ├── closure_library/
│       │   ├── closure/
│       │   │   ├── bin/
│       │   │   │   ├── build/
│       │   │   │   │   ├── __init__.py*
│       │   │   │   │   ├── source.py
│       │   │   │   │   └── treescan.py
│       │   │   │   └── __init__.py*
│       │   │   └── __init__.py*
│       │   └── __init__.py*
│       └── __init__.py*
└── MANIFEST*

@damienmg
Copy link
Contributor Author

damienmg commented Apr 1, 2016

It should work or we should fix bazel

On Fri, Apr 1, 2016 at 5:10 PM Justine Tunney [email protected]
wrote:

Are you sure your fix works for Python? It appears that the generated
Python runner script does:

external_dir = os.path.join(module_space, 'external')
print external_dir
if os.path.isdir(external_dir):
external_entries = [os.path.join(external_dir, d) for d in os.listdir(external_dir)]
repositories = [d for d in external_entries if os.path.isdir(d)]
print repositories
python_path_entries += repositories

But the generated structure is now:

./bazel-bin/closure/compiler/jschecker/jschecker.runfiles
├── io_bazel_rules_closure/
│ ├── closure/
│ │ ├── compiler/
│ │ │ ├── init.py*
│ │ │ └── jschecker/
│ │ │ ├── init.py*
│ │ │ ├── jschecker
│ │ │ └── jschecker.py
│ │ └── init.py*
│ └── external/
│ ├── closure_library/
│ │ ├── closure/
│ │ │ ├── bin/
│ │ │ │ ├── build/
│ │ │ │ │ ├── init.py*
│ │ │ │ │ ├── source.py
│ │ │ │ │ └── treescan.py
│ │ │ │ └── init.py*
│ │ │ └── init.py*
│ │ └── init.py*
│ └── init.py*
└── MANIFEST*


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#32 (comment)

@jart
Copy link
Contributor

jart commented Apr 1, 2016

Perhaps Bazel needs to be fixed. You can reproduce this error by cloning the Closure Rules and applying the following change:

diff --git a/closure/compiler/jschecker/jschecker.py b/closure/compiler/jschecker/jschecker.py
index 113b534..a21547f 100644
--- a/closure/compiler/jschecker/jschecker.py
+++ b/closure/compiler/jschecker/jschecker.py
@@ -44,8 +44,8 @@ import os
 import re
 import sys

-from external.closure_library.closure.bin.build import source
-from external.closure_library.closure.bin.build import treescan
+from closure.bin.build import source
+from closure.bin.build import treescan

 def _get_options_parser():

@damienmg
Copy link
Contributor Author

damienmg commented Apr 1, 2016

ok that's because of bazelbuild/bazel#1040

@jart
Copy link
Contributor

jart commented Apr 1, 2016

When these issues are fixed, would it be possible for you guys to roll a new release? Otherwise my Travis build is going to be broken and I can't make good recommendations to users.

@damienmg
Copy link
Contributor Author

damienmg commented Apr 1, 2016

Yes it would be but I think it is not needed. Fixing the issue on our side should make your code work without any change to the closure_rules themselves.

btw, while looking at your code I found the comment on the closure_library. You could use a custom skylark repository rule to generate a http archive with multiple build file (and there you can use labels even with 0.2.1, no need to wait for next release)

@jart
Copy link
Contributor

jart commented Apr 1, 2016

What issue are you fixing exactly? I'd like to get rid of the external.closure_library prefix. I've also got a whole bunch of other stuff that explicitly references the external/foo/ structure in runfiles. I'd like to switch over to the new structure as soon as possible. I don't think it's going to be possible to maintain backwards compatibility.

@damienmg
Copy link
Contributor Author

damienmg commented Apr 1, 2016

The import path of python point to the wrong location.

With Bazel head it points to the correct location but you still need external.closure_library prefix because the package name clashes with your main repository (all top level python package is closure)

The fix will add the io_bazel_... prefix to all python import. I am trying to bisect to see exactly what caused that behavior.

We have a long term plan to make everything a bit more logical but it is still in its early phase.

@damienmg
Copy link
Contributor Author

damienmg commented Apr 1, 2016

Ok what caused that behavior is fixing using the workspace name... so the breakage come from actually having a name in the workspace name...

@jart
Copy link
Contributor

jart commented Apr 1, 2016

The workspace name is good. I want it to put stuff in that io_bazel_rules_closure subdirectory, due to the issue I raised in bazelbuild/bazel/issues/1090.

@damienmg
Copy link
Contributor Author

damienmg commented Apr 1, 2016

Yes I agree, but we should have the other repo in the runfiles base:

.runfiles/
  closure_library/
  closure_compiler/
  io_bazel_closure_rules/

not the way it is:

.runfiles/
   io_bazel_closure_rules/
      external/
         closure_library/
         closure_compiler/

Anyway, no action is required from the closure_rules to fix the issue. I think we will have a fix on monday on Bazel and hopefully we will roll out the new symlink structure soon enough for closure rules :)

@damienmg damienmg closed this as completed Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants