Skip to content

ci: use github actions#2622

Merged
patak-cat merged 17 commits intovitejs:mainfrom
Shinigami92:issue-2604
Apr 10, 2021
Merged

ci: use github actions#2622
patak-cat merged 17 commits intovitejs:mainfrom
Shinigami92:issue-2604

Conversation

@Shinigami92
Copy link
Copy Markdown
Member

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Description

#2604

This adds a CI script to the repo so GitHub Actions are used
This will provide warnings from eslint inside reviews in the GUI of GitHub
Also it may replace CircleCI in the long run

Additional context

I have added also some outcommented steps that are arguable if we should use them
In my smaller repos these are quite okay, but for such a big sized repo as Vite it's really questionable

@Shinigami92 Shinigami92 added 🔍 review needed p1-chore Doesn't change code behavior (priority) labels Mar 21, 2021
@Shinigami92 Shinigami92 requested a review from patak-cat March 21, 2021 14:34
@Shinigami92
Copy link
Copy Markdown
Member Author

Shinigami92 commented Mar 21, 2021

👀 The tests on windows seems to fail successfully hihi 🤭
So I think I discovered a real issue by accident

@patak-cat
Copy link
Copy Markdown
Member

I think this is a good idea, but I have not enough experience with GitHub actions to review this one. Let's wait for @antfu and @underfin's input

aminya
aminya previously approved these changes Mar 23, 2021
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
@Shinigami92 Shinigami92 marked this pull request as draft March 23, 2021 14:24
Shinigami92 and others added 2 commits March 23, 2021 15:32
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
@aminya
Copy link
Copy Markdown
Contributor

aminya commented Mar 23, 2021

You should also probably remove CircleCI and Appveyor to stop them from testing something that is already being tested by GitHub Actions. This prevents wasting energy that is consumed for running those extra virtual machines.

CircleCI has a "stop building" button in its settings, and Appveyor can be turned off from the repository settings.

If you don't have access to these, you can do some tricks. For Appveyor for example:

# empty appveyor
build: off

branches:
  only:
  - non-existing

@Shinigami92
Copy link
Copy Markdown
Member Author

You should also probably remove CircleCI and Appveyor [...]

I would prefer to leave this to the core team, maybe in an additional PR

@Shinigami92 Shinigami92 marked this pull request as ready for review March 23, 2021 14:50
aminya
aminya previously approved these changes Mar 23, 2021
Copy link
Copy Markdown
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

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

Looks good to me!

The errors in the failing tests on Windows are irrelevant to these changes and should probably be fixed in a separate PR.

@Shinigami92
Copy link
Copy Markdown
Member Author

The errors in the failing tests on Windows are irrelevant to these changes and should probably be fixed in a separate PR.

=> #2631

@Shinigami92 Shinigami92 requested a review from GrygrFlzr April 8, 2021 15:13
@Shinigami92
Copy link
Copy Markdown
Member Author

Shinigami92 commented Apr 8, 2021

WHAT 👀
https://github.com/vitejs/vite/pull/2622/checks?check_run_id=2298098539#step:8:31

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 019FE94A v8::internal::Heap::PageFlagsAreConsistent+3050
 2: 019F50B0 v8::internal::Heap::CollectGarbage+1200
error Command failed with exit code 134.

I think it's coming from this change:

fcd6497#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR23

- node_version: 12
+ node: 12 

Indeed, previously it was wrong and falled-back to default node 14: https://github.com/vitejs/vite/runs/2298014808#step:4:7
And now it was corrected to node 12 like suggested: https://github.com/vitejs/vite/pull/2622/checks?check_run_id=2298098539#step:4:7

@Shinigami92
Copy link
Copy Markdown
Member Author

The error message now look different 🤔

https://github.com/vitejs/vite/pull/2622/checks?check_run_id=2298256877#step:8:31

<--- Last few GCs --->

[5388:039FC9C8]    66391 ms: Mark-sweep (reduce) 1021.0 (1027.0) -> 1019.6 (1028.0) MB, 1545.6 / 0.0 ms  (average mu = 0.099, current mu = 0.022) allocation failure scavenge might not succeed
[5388:039FC9C8]    67785 ms: Mark-sweep (reduce) 1022.1 (1030.0) -> 1019.4 (1029.0) MB, 1388.3 / 0.0 ms  (average mu = 0.053, current mu = 0.004) allocation failure scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0086E5C9 v8::internal::Heap::PageFlagsAreConsistent+2169
 2: 00864651 v8::internal::Heap::CollectGarbage+1985
 3: 00862D03 v8::internal::Heap::AllocateExternalBackingStore+1155

@Shinigami92 Shinigami92 marked this pull request as draft April 8, 2021 16:19
@Shinigami92
Copy link
Copy Markdown
Member Author

Shinigami92 commented Apr 8, 2021

Sorry @aminya but we had some GH Action issues with windows-2016_x86 due to out of memory while testing
Therefore we decided to just kick it 🤷
We want to have a bit more simple CI

@Shinigami92 Shinigami92 marked this pull request as ready for review April 8, 2021 16:58
@aminya
Copy link
Copy Markdown
Contributor

aminya commented Apr 9, 2021

I think the scope of the issue is larger than this pull request, but it seems like a genuine issue in the native code. Doesn't Esbuild support Windows x86?

- name: Test build
run: yarn test-build --runInBand

lint:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to have this separately? The installing/build processes are kinda duplicated to me, maybe we can have it inside the build job with job.continue-on-error to have both lint errors and test errors, and also job.if make it only run once.

Copy link
Copy Markdown
Contributor

@aminya aminya Apr 10, 2021

Choose a reason for hiding this comment

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

You can shrink this if you want.

    runs-on: ubuntu-latest
    name: 'Lint'
    steps:
      - uses: actions/checkout@v2
      - run: |
          yarn install --frozen-lockfile
          yarn build-vite
          yarn build-plugin-vue
      - run: yarn lint

I would put those yarn install and build stuff into the prepare so they run with yarn install

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@aminya

I personally like it more clean and all have set explicit name
But if really wanted I can omit names here and there

Also I want to set node to version 14 explicitly, so that if actions/setup-node@v2 / GitHub Actions would get an update, we have the control of what to update when.


@antfu

Having it separately makes a bit sense here, cause the job itself is specialized for doing one thing and that also quickly

Separating the jobs like this also helps a bit to jump into e.g. special test errors. So not everything is made at one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p1-chore Doesn't change code behavior (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants