Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Replace AppVeyor with Travis CI for Windows #49

Merged
merged 2 commits into from
Sep 23, 2019
Merged

Conversation

vince-fugnitto
Copy link
Member

  • Replaces AppVeyor with Travis CI for Windows
  • Remove appveyor.yml
  • Update travis.yml to include os: windows build.
  • Add badges for gitpod, travis and bugs.

Signed-off-by: Vincent Fugnitto [email protected]

@vince-fugnitto
Copy link
Member Author

With this change, we now get the badges present on the README:

Screen Shot 2019-09-13 at 1 06 13 PM

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @vince-fugnitto

@vince-fugnitto
Copy link
Member Author

There seems to be an issue with the windows build.
After discussion with @marechal-p it looks to be an issue with yarn and what it outputs. (yarnpkg/yarn#4564)

As you can see the following line:
Error: Cannot find module 'C:\Users\travis\build\eclipse-theia\theia-cpp-extensions\node_modules\node_modules\@theia\debug\bin\download-adapters.js'
Contains node_modules twice.

@lmcbout
Copy link
Contributor

lmcbout commented Sep 20, 2019

@vince-fugnitto Are you planning to add the preinstall to fix the window compilation
yarnpkg/yarn#4564

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto Are you planning to add the preinstall to fix the window compilation
yarnpkg/yarn#4564

If I do I'll require a CQ to copy the code from someone who's done it, however it does look hackish, the real fix should be done at yarn.

@marcdumais-work
Copy link
Contributor

If I do I'll require a CQ to copy the code from someone who's done it, however it does look hackish, the real fix should be done at yarn.

Yes, that really looks like a hack. Having windows CI for this repo is not pressing - let's wait to see if we get permission to fix the issue in yarn.

@vince-fugnitto
Copy link
Member Author

If I do I'll require a CQ to copy the code from someone who's done it, however it does look hackish, the real fix should be done at yarn.

Yes, that really looks like a hack. Having windows CI for this repo is not pressing - let's wait to see if we get permission to fix the issue in yarn.

With @marechal-p help we were able to find a workaround, waiting for the CI to finish to see if it successfully worked :)

@vince-fugnitto
Copy link
Member Author

@lmcbout @marcdumais-work
It looks like all three operating systems (linux, macOS, windows) now passed using the workaround, is it fine to merge?

@lmcbout
Copy link
Contributor

lmcbout commented Sep 20, 2019

Your workaround does not come from the patch, you apply a postinstall and not the preinstalled as defined in the other PR ( yarnpkg/yarn#4564 ). If we wait for yarn to be fixed, it might take a while. This problem was reported 2 years ago and still not fixed, so the workaround sounds very good to me.
One thing if the file download-adapters.js is not there for any reasons, can we check for it and report and error to the end-user !

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Sep 20, 2019

Your workaround does not come from the patch, you apply a postinstall and not the preinstalled as defined in the other PR ( yarnpkg/yarn#4564 ).

There is no other fix as far as I know, the link you mentioned is an issue not a PR.

One thing if the file download-adapters.js is not there for any reasons, can we check for it and report and error to the end-user !

The download-adapters.js script is defined in @theia/debug dependency.
The command will fail and report the error which I believe is enough of a report to end-users/developers. What more can we do?

Update: (performing yarn postinstall before yarn)

yarn run v1.17.3
$ node -e "require('@theia/debug/bin/download-adapters.js')"
internal/modules/cjs/loader.js:584
    throw err;
    ^

Error: Cannot find module '@theia/debug/bin/download-adapters.js'

I believe this is sufficient, I don't see what more we could do in such a case.

- Replaces `AppVeyor` with `Travis CI` for Windows
- Remove `appveyor.yml`
- Update `travis.yml` to include `os: windows` build.
- Add badges for `gitpod`, `travis` and `bugs`.

Signed-off-by: Vincent Fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

Updated the PR to split the changes into 2 commits:

  1. replace appveyor with travis ci for windows
  2. update the postinstall dowload-debug-adapters script for both cortex-debug and cpp-debug

@vince-fugnitto vince-fugnitto force-pushed the vf/travis-win branch 2 times, most recently from 73eb521 to d315333 Compare September 20, 2019 18:27
- updated the `postinstall` scripts for both `cortext-debug` and `cpp-debug`.
- updated the scripts as a workaround to a known yarn issue on windows
(yarnpkg/yarn#4564)

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto vince-fugnitto merged commit b1b1e56 into master Sep 23, 2019
@vince-fugnitto vince-fugnitto deleted the vf/travis-win branch September 23, 2019 15:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants