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

Streamline how we build CodeMirror in dist/ so things get bundled, part of #634 #636

Merged
merged 3 commits into from
Mar 14, 2017

Conversation

humphd
Copy link

@humphd humphd commented Mar 8, 2017

This cleans up how we do CodeMirror in dist/ so that everything gets pre-bundled in our main requirejs bundle. The only thing we then need to have in dist/ is a CSS file.

@humphd humphd requested a review from gideonthomas March 8, 2017 20:18
@humphd
Copy link
Author

humphd commented Mar 10, 2017

I don't understand why travis is failing this, it all passes locally.

@Pomax
Copy link

Pomax commented Mar 11, 2017

This looks like the dreaded NaN dependency error, which traditionally is caused by using the wrong version of Node.js for the version of nan that is being used

@humphd
Copy link
Author

humphd commented Mar 11, 2017

Based on that, I see:

npm WARN unmet dependency /Users/dave/Sites/repos/brackets/node_modules/grunt-contrib-requirejs requires requirejs@'~2.1.0' but will load
npm WARN unmet dependency /Users/dave/Sites/repos/brackets/node_modules/requirejs,
npm WARN unmet dependency which is version 2.3.3
[email protected] /Users/dave/Sites/repos/brackets
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected]
└─┬ [email protected]
  └─┬ [email protected]
    └── [email protected]

What do you think?

@Pomax
Copy link

Pomax commented Mar 13, 2017

Digging through the logs again it looks like Travis is actually using an incompatible C++ compiler.

> [email protected] install /home/travis/build/mozilla/brackets/node_modules/iltorb
> node-gyp rebuild
gyp WARN download NVM_NODEJS_ORG_MIRROR is deprecated and will be removed in node-gyp v4, please use NODEJS_ORG_MIRROR
gyp WARN download NVM_NODEJS_ORG_MIRROR is deprecated and will be removed in node-gyp v4, please use NODEJS_ORG_MIRROR
gyp WARN download NVM_NODEJS_ORG_MIRROR is deprecated and will be removed in node-gyp v4, please use NODEJS_ORG_MIRROR
make: Entering directory `/home/travis/build/mozilla/brackets/node_modules/iltorb/build'
  CC(target) Release/obj.target/decode/brotli/common/dictionary.o
  CC(target) Release/obj.target/decode/brotli/dec/bit_reader.o
  CC(target) Release/obj.target/decode/brotli/dec/decode.o
  CC(target) Release/obj.target/decode/brotli/dec/huffman.o
  CC(target) Release/obj.target/decode/brotli/dec/state.o
  CXX(target) Release/obj.target/decode/src/common/allocator.o
In file included from ../src/common/allocator.cc:2:0:
../../nan/nan.h:43:3: error: #error This version of node/NAN/v8 requires a C++11 compiler

Did a bit of digging and the best I can find is this post: https://sudogem.wordpress.com/2016/06/14/travis-ci-this-version-of-nodenanv8-requires-a-c11-compiler/

Which has instructions on how to tell Travis which compiler to use:

language: node_js
node_js: stable
before_install:
  - sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test
  - sudo apt-get -qq update
  - sudo apt-get -qq install g++-4.8
env:
  - CXX=g++-4.8

@humphd
Copy link
Author

humphd commented Mar 13, 2017

Sounds like #592, which maybe I'll do first.

@humphd humphd force-pushed the codemirror-minify branch from f47764e to 94ee71a Compare March 13, 2017 19:42
@humphd
Copy link
Author

humphd commented Mar 13, 2017

I've landed #592, which will hopefully fix the travis build problem, and rebased this.

@humphd
Copy link
Author

humphd commented Mar 14, 2017

OK, this is ready to review, I've fixed all the failures.

@Pomax
Copy link

Pomax commented Mar 14, 2017

npm install --no-optional + npm run build succeeds, npm start succeeds, but accessing in the browser than yields this error:

screenshot 199

Hitting up "hosted" then seems to load things without problems, but the console still has a lot of data in it that should either be cleaned up or fixed (probably not in this PR, though)

@humphd
Copy link
Author

humphd commented Mar 14, 2017

NOTE: you can't load it like this. Try loading http://localhost:8000/dist/hosted.html for the built version and http://localhost:8000/src/hosted.html for the source version.

@Pomax
Copy link

Pomax commented Mar 14, 2017

quite, but that error is new nonetheless. hosted works fine so I think this is good to merge

@humphd humphd merged commit 4426127 into mozilla:master Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants