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

Added continuous integration for windows #318

Merged

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Apr 14, 2020

I added continuous integration for windows.

  • Rename Check to Check on Linux
  • Added Check on Windows
  • Rename Test Suite to Test Suite on Linux
  • Added Test Suite on Windows

I did not add Rust Fmt on Windows and Rust Clippy on Windows, since there are not os specific.

what do you think? @Razican @jasonwilliams

Also should I add for benchmark too?

@HalidOdat HalidOdat added enhancement New feature or request help wanted Extra attention is needed labels Apr 14, 2020
@Razican
Copy link
Member

Razican commented Apr 14, 2020

Could be interesting to add benchmarks, yes. And I think we should have at least one benchmark of both lexing and parsing, that could be checked in the future when we merge both. We can take this opportunity to do that.

I would also add a benchmark of a bit more complex program too than a hello world.

@Razican
Copy link
Member

Razican commented Apr 14, 2020

Oh, and I think we should upgrade the checkout action to v2. API is the same, but it's faster.

@Razican Razican added this to the v0.8.0 milestone Apr 14, 2020
@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 14, 2020

Oh, and I think we should upgrade the checkout action to v2. API is the same, but it's faster.

I actually did this but it caused problems so I had to revert.
Basically v2 only checks out the branch the action is running on, so when benchmarks switch to master it fails, v1 was more like “git pull”.

I’m sure this is fixable, but if you hit the same issue that seems to be what it is.
I’m guessing with v2 you need to pass some flag to say you want the other branches too or master included. Either that or it’s some weird bug.

For the CI workflow v2 should work fine as there’s no branch switching happening

@HalidOdat HalidOdat marked this pull request as ready for review April 15, 2020 21:41
@HalidOdat
Copy link
Member Author

I upgraded CI to checkout@v2 and it seems to be working fine.

I think this is ready to merge. :)

@Razican
Copy link
Member

Razican commented Apr 16, 2020

I think it would be interesting to benchmark on Windows too, since for example Jemallocator is Linux-only.

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 16, 2020

I think it would be interesting to benchmark on Windows too, since for example Jemallocator is Linux-only.

I think we should do this in another PR. :)

BTW I'm not very knowledgeable of how the benchmarks are set or github actions for that matter.

Edit: I can add in CI a step to see if benchmark compiles on Linux and Windows

@jasonwilliams
Copy link
Member

+1 benchmarking being done in a separate PR

@Razican Razican merged commit 7dd32a6 into boa-dev:master Apr 16, 2020
@HalidOdat HalidOdat deleted the feature/continuous-integration-windows branch April 16, 2020 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants