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

[onhold] GH-2009: Bumped up a few versions to support Node.js 10.x. #3358

Closed
wants to merge 3 commits into from

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Oct 31, 2018

For reviewers:

  • It does not mean we switch to Node.js 10.
  • This update just makes sure, we can build Theia on Node.js 10.

  • The blocker was in vscode-nsfw due to nsfw. 1.1.1 had the fix.
  • Bumped up the transitive fiber dependency (via wdio) as well.

TODOs:

  • Verify the file watcher behavior.
    • File modification.
    • File deletion.
    • File creation.
  • Run the tests with Node.js 10.
  • TBD

Closes: #2009
Signed-off-by: Akos Kitta [email protected]

Edit: added TODOs.

Akos Kitta added 2 commits October 31, 2018 17:36
 - The blocker was in `vscode-nsfw` due to `nsfw`. `1.1.1` had the fix.
 - Bumped up the transitive `fiber` dependency (via `wdio`) as well.

Closes: #2009
Signed-off-by: Akos Kitta <[email protected]>
This should not get to `master` for GH-2009.
@kittaakos kittaakos changed the title [WIP] GH-2009: Bumped up a few versions to support Node.js 10.x. GH-2009: Bumped up a few versions to support Node.js 10.x. Oct 31, 2018
.travis.yml Show resolved Hide resolved
@kittaakos
Copy link
Contributor Author

I could not build on Windows with Node.js 10.0.0 but with 10.13.0:

C:\Users\kittaakos\dev\theia\node_modules\find-git-repositories\openpa\src\opa_queue.h(59): note: see declaration of '
OPA_Shm_rel_addr_t' (compiling source file ..\src\FindGitRepos.cpp)
..\src\FindGitRepos.cpp(67): warning C4554: '&': check operator precedence for possible error; use parentheses to clarif
y precedence [C:\Users\kittaakos\dev\theia\node_modules\find-git-repositories\build\findGitRepos.vcxproj]
..\src\FindGitRepos.cpp(68): warning C4554: '&': check operator precedence for possible error; use parentheses to clarif
y precedence [C:\Users\kittaakos\dev\theia\node_modules\find-git-repositories\build\findGitRepos.vcxproj]
..\src\FindGitRepos.cpp(107): warning C4996: 'Nan::Callback::Call': was declared deprecated [C:\Users\kittaakos\dev\thei
a\node_modules\find-git-repositories\build\findGitRepos.vcxproj]
  C:\Users\kittaakos\dev\theia\node_modules\nan\nan.h(1618): note: see declaration of 'Nan::Callback::Call'
..\src\FindGitRepos.cpp(131): warning C4996: 'Nan::Callback::Call': was declared deprecated [C:\Users\kittaakos\dev\thei
a\node_modules\find-git-repositories\build\findGitRepos.vcxproj]
  C:\Users\kittaakos\dev\theia\node_modules\nan\nan.h(1618): note: see declaration of 'Nan::Callback::Call'
LINK : fatal error LNK1104: cannot open file 'C:\Users\kittaakos\.node-gyp\10.0.0\x64\node.lib' [C:\Users\kittaakos\dev\
theia\node_modules\find-git-repositories\build\findGitRepos.vcxproj]
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\MSBuild\14.0\bin\msbuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Users\kittaakos\dev\theia\node_modules\node-gyp\lib\build.js:262:23)
gyp ERR! stack     at ChildProcess.emit (events.js:182:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:225:12)
gyp ERR! System Windows_NT 10.0.17134
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\kittaakos\\dev\\theia\\node_modules\\node-gyp\\bin\\n
ode-gyp.js" "rebuild"

That should be `10.11.0` according to AppVeyor.
@kittaakos
Copy link
Contributor Author

For some reason the Windows build is failing with some c++ UI test.

[chrome #0-0] Session ID: b1b6c19fda3b77b09fc5f098c894aa39
4422[chrome #0-0] Spec: C:\projects\theia\examples\browser\test\cpp\cpp-change-build-config.ui-spec.ts
4423[chrome #0-0] Running: chrome
4424[chrome #0-0]
4425[chrome #0-0] cpp extension
4426[chrome #0-0]   1) should be able to change build config
4427[chrome #0-0]
4428[chrome #0-0]
4429[chrome #0-0] 1 failing (1m, 2s)
4430[chrome #0-0]
4431[chrome #0-0] 1) cpp extension should be able to change build config:
4432[chrome #0-0] Timeout of 30000ms exceeded. Try to reduce the run time or increase your timeout for test specs (http://webdriver.io/guide/testrunner/timeouts.html); if returning a Promise, ensure it resolves. (C:\projects\theia\examples\browser\test\cpp\cpp-change-build-config.ui-spec.ts)
4433[chrome #0-0] Error: Timeout of 30000ms exceeded. Try to reduce the run time or increase your timeout for test specs (http://webdriver.io/guide/testrunner/timeouts.html); if returning a Promise, ensure it resolves. (C:\projects\theia\examples\browser\test\cpp\cpp-change-build-config.ui-spec.ts)
4434[chrome #0-0]     at Test.Runnable._timeoutError (C:\projects\theia\node_modules\wdio-mocha-framework\node_modules\mocha\lib\runnable.js:440:10)
4435[chrome #0-0]     at Timeout.<anonymous> (C:\projects\theia\node_modules\wdio-mocha-framework\node_modules\mocha\lib\runnable.js:251:24)
4436[chrome #0-0]     at ontimeout (timers.js:425:11)
4437[chrome #0-0]     at tryOnTimeout (timers.js:289:5)
4438[chrome #0-0]     at listOnTimeout (timers.js:252:5)
4439[chrome #0-0]     at Timer.processTimers (timers.js:212:10)
4440[chrome #0-0]
4441

Could this be related to the Node.js version, @simark?

@marcdumais-work
Copy link
Contributor

@kittaakos

For some reason the Windows build is failing with some c++ UI test.

It seems that the same issue or similar happens on Linux:

https://travis-ci.org/theia-ide/theia/jobs/449234722#L7936

We'll need to investigate

@marcdumais-work
Copy link
Contributor

same issue or similar happens on Linux

I can reproduce when I run the UI tests locally on node v10.13.0

@kittaakos
Copy link
Contributor Author

We'll need to investigate

OK. Maybe it is a test configuration issue with the latest wdio. It did pass previously.

screen shot 2018-11-01 at 07 54 22
screen shot 2018-11-01 at 07 54 08

The most recent commit just pushed up the Node.js version to 10.11.x for AppVeyor. I won't have time adjusting the Chrome drivers just to make flaky tests pass. I add the "help wanted" label to the ticket.

@kittaakos kittaakos changed the title GH-2009: Bumped up a few versions to support Node.js 10.x. [onhold] GH-2009: Bumped up a few versions to support Node.js 10.x. Nov 1, 2018
@marcdumais-work
Copy link
Contributor

marcdumais-work commented Nov 1, 2018

It did pass previously.

The failing cpp UI test that's failing is newly merged from yesterday - it may not have been in your baseline when the CI for this PR here previously passed. These tests were passing consistently on node v8.x.

@kittaakos
Copy link
Contributor Author

These tests were passing consistently on node v8.x.

See a completely unrelated PR; it fails with the same issue C++ issue: https://ci.appveyor.com/project/kittaakos/theia/builds/19967480

@marcdumais-work
Copy link
Contributor

See a completely unrelated PR; it fails with the same issue C++ issue:

Thanks for pointing this out. We'll look into it.

@marcdumais-work
Copy link
Contributor

quick update on a previous post:

same issue or similar happens on Linux

I can reproduce when I run the UI tests locally on node v10.13.0

Seems I was a bit hasty: the failure I saw locally was not the same, but rather caused by switching node version, without doing a good cleaning. After a git clean -fdx and rebuilding theia, now the UI tests pass locally for me, on Linux, using node 10.x.

@kittaakos
Copy link
Contributor Author

Thanks for the verification, @marcdumais-work. We can get back to this later, it is not a blocker. I think Node.js 10 won't be a problem in general, we might have test configuration issues due to wdio.

@simark
Copy link
Contributor

simark commented Nov 1, 2018

The cpp test does fail consistently on AppVeyor it seems, I suggest disabling it on Windows for now. See #3365.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Nov 1, 2018

@kittaakos something seems weird. Maybe you can confirm if you see this too or not on your local machine...

In the most recent Travis CI log, we see the cpp UI test failure: https://travis-ci.org/theia-ide/theia/jobs/449234722#L7568 :

[...]
@theia/example-browser: [chrome #0-0] Spec: /home/travis/build/theia-ide/theia/examples/browser/test/cpp/cpp-change-build-config.ui-spec.ts
[...]

However, I do not see that file or its containing cpp folder, on my local checkout of this PR. Could it be that somehow Travis is leaving behind some files from a previous build / PRs?

$ git status
On branch GH-2009
Your branch is up to date with 'upstream/GH-2009'.

nothing to commit, working tree clean

$ git log --oneline
b6df25aa (HEAD -> GH-2009, upstream/GH-2009) [DROP-ME] Switched from Node.js 10 to Current.
4fa096fe [DROP-ME] GH-2009: Switched to Node.js 10 on the CIs.
1e12147c GH-2009: Bumped up a few versions to support Node.js `10.x`.
ca336686 GH-3348: Added a snippet for the copyright header.
7703efd0 Fix quote style in (mostly) test files
[...]

$ ls -al examples/browser/test/cpp
ls: cannot access 'examples/browser/test/cpp': No such file or directory
$ grep -R "cpp-change-build-config.ui-spec.ts" *
$
$ git checkout master
Switched to branch 'master'
$ ll examples/browser/test/cpp
total 16
drwxr-xr-x 2 lmcmcds eusers 4096 Nov  1 09:42 ./
drwxr-xr-x 8 lmcmcds eusers 4096 Nov  1 09:42 ../
-rw-r--r-- 1 lmcmcds eusers 4993 Nov  1 09:42 cpp-change-build-config.ui-spec.ts

@kittaakos
Copy link
Contributor Author

Could it be that somehow Travis is leaving behind some files from a previous build / PRs?

Exactly. I had the same feeling, maybe it is caching something. I do not have the CPP test module locally either.

The failing cpp UI test that's failing is newly merged from yesterday - it may not have been in your baseline when the CI for this PR here previously passed.

I did not rebase, so I was surprised why did start to fail. Shall we clean the Travis/AppVeyro caches?

@marcdumais-work
Copy link
Contributor

Shall we clean the Travis/AppVeyro caches?

@vince-fugnitto already did it for Travis. Could you do the same for AppVeyor?

@marcdumais-work
Copy link
Contributor

I did not rebase, so I was surprised why did start to fail. Shall we clean the Travis/AppVeyro caches?

@simark and I noticed something, but we are not sure if it's new or we just never noticed before. It looks like Travis and Appveyor do not test the exact commit from our PR. Instead they seem to clone master, merge the commit-under-test, into master, and then test the result (if no merge conflict). This would explain why we see Simon's changes in this not-yet-uplifted PR.

e.g.: https://travis-ci.org/theia-ide/theia/jobs/449234722#L556

Are we crazy or did the CI not work like this before?

@simark
Copy link
Contributor

simark commented Nov 1, 2018

@simark and I noticed something, but we are not sure if it's new or we just never noticed before. It looks like Travis and Appveyor do not test the exact commit from our PR. Instead they seem to clone master, merge the commit-under-test, into master, and then test the result (if no merge conflict). This would explain why we see Simon's changes in this not-yet-uplifted PR.

e.g.: https://travis-ci.org/theia-ide/theia/jobs/449234722#L556

To be clear, if you follow that link, it's not Travis or AppVeyor that do this merge, they just pull a special branch provided by github, which is the result of merging the branch under test in the target branch:

git fetch origin +refs/pull/3358/merge:

@kittaakos kittaakos closed this Nov 6, 2018
@kittaakos kittaakos deleted the GH-2009 branch November 6, 2018 09:49
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.

Theia does not run with Node.js v10.3.0
4 participants