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

Remove deprecated Python Closure Linter #55

Merged
merged 1 commit into from
May 12, 2016

Conversation

jart
Copy link
Contributor

@jart jart commented May 11, 2016

This fixes 50% of #52.

CC @damienmg @hochhaus

@hochhaus
Copy link
Contributor

LGTM.

As part of a different change, what do you think of adding a rule to enforce formatting via clang-format? The compiler lint checks are relatively loose on formatting so I believe that closure-library uses clang-format.

@jart
Copy link
Contributor Author

jart commented May 12, 2016

Wow it uses Clang?!

I wish they had chosen something written in Java because the Clang format dependency is 107MB (http://llvm.org/releases/3.7.1/clang+llvm-3.7.1-x86_64-linux-gnu-ubuntu-14.04.tar.xz) and 557MB unzipped. Although the clang-format program itself appears to be 1.5MB. It also doesn't appear to dynamically link against anything weird:

master jart@compy:~/code/closure-library$ ldd ./clang+llvm-3.7.1-x86_64-linux-gnu-ubuntu-14.04/bin/clang-format 
        linux-vdso.so.1 =>  (0x00007fff561f2000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fef910ef000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fef90eeb000)
        libtinfo.so.5 => /lib/x86_64-linux-gnu/libtinfo.so.5 (0x00007fef90cc2000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fef90aa4000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fef9088b000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fef90585000)
        libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fef90281000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fef9006b000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fef8fca6000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fef912f7000)

So it might be possible to ask tar to only extract that single file. Then it should hopefully just work in a hermetically sealed linux build environment.

The problem is they don't appear to have a darwin release, so we might not be able to maintain Mac compatibility, in a manner similar to how Closure Rules currently maintains Mac compatibility for PhantomJS.

If you want to investigate further, be my guest. But I feel we'd probably be much better off is functionality of relative parity could be offered via Java. Possibly by finding someone willing to adapt google-java-format for JS.

@jart jart merged commit 626db8f into bazelbuild:master May 12, 2016
@hochhaus
Copy link
Contributor

hochhaus commented May 12, 2016

Thanks for your comments. I agree that clang is a really heavy weight dependency just for formatting.

Your proposed java based solution is good as well.

My only additional feedback is that llvm does make apple releases (for example 3.7.0 and 3.8.0). I'm not sure why it is missing from the 3.7.1 download directory. Therefore Mac compatibility should be possible to retain even if we relied on clang-format.

I will look into this more as I have time.

@jart jart deleted the remove-old-linter branch May 29, 2016 21:09
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

3 participants