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 support for Apple Silicon. Fixes #1997 #1998

Merged
merged 3 commits into from
May 21, 2021

Conversation

waldemar-kindler
Copy link
Contributor

@waldemar-kindler waldemar-kindler commented May 15, 2021

This PR adds support for the Apple Silicon.

I'm not familiar with the different packaging options you are providing. Would really appreciate any hints what else needs to be changed.

@sriv
Copy link
Member

sriv commented May 19, 2021

Hi @Bonitas - thanks for setting this up.

the DCO check is failing because one or more commits are not signed, please see the notes here: https://github.com/getgauge/gauge/pull/1998/checks?check_run_id=2609216859

As for packaging for macOS, the below options are available:

  • go get install - nothing to be done, golang's tooling should handle this
  • brew install - nothing to be done here AFAIK, the homebrew team package and distribute the bottles, so it should work.
  • npm install - I see you've already added a commit for this.
  • pip install - same as npm, I see changes to include this.
  • zip install - It should be addressed since you've added arm to getPackageArchSuffix() in make.go
  • codesign - needs testing, and will need to check if github actions has support for this.

tldr - I need to check about codesign. everything else seems to be in place.

@waldemar-kindler
Copy link
Contributor Author

waldemar-kindler commented May 19, 2021

Hi @sriv,

Thanks for your feedback.

Would you prefer me to rebase sll commits into one for the DCO? Otherwise I think I'll need to edit the merge commit. Thats the one where I missed the sign.

Also maybe it's worth to add arm support for linux. Didn't touch this as I had no possibility to test that. Alternatively we could throw an error to make it explicit that this is currently unsupported.

@sriv
Copy link
Member

sriv commented May 20, 2021

hi @Bonitas

Would you prefer me to rebase sll commits into one for the DCO? Otherwise I think I'll need to edit the merge commit. Thats the one where I missed the sign.

Yes, a rebase should be fine in this case.

Also maybe it's worth to add arm support for linux

Yes, the change should be quite small, but it's the testing that I am worried about too. Let's defer this until someone finds a need for it.

@github-actions
Copy link
Contributor

github-actions bot commented May 20, 2021

Benchmark Results

java_simple_parallel.csv

Commit CPU Memory Time ExitCode
6e0d83f 185% 316576 0:20.36 0
c532320 188% 316540 0:20.17 0
c93f7af 182% 324672 0:20.33 0
d5b8616 176% 318292 0:20.55 0

java_maven_multithreaded.csv

Commit CPU Memory Time ExitCode
6e0d83f 183% 355184 0:23.08 0
c532320 183% 303728 0:22.61 0
c93f7af 153% 362648 0:37.51 0
d5b8616 159% 312072 0:30.12 0

java_simple_multithreaded.csv

Commit CPU Memory Time ExitCode
6e0d83f 181% 397856 0:13.15 0
c532320 174% 384524 0:13.73 0
c93f7af 174% 385536 0:14.13 0
d5b8616 168% 347744 0:15.96 0

java_gradle_parallel.csv

Commit CPU Memory Time ExitCode
6e0d83f 178% 394976 0:20.54 0
c532320 178% 374068 0:20.26 0
c93f7af 166% 639544 0:30.80 0
d5b8616 182% 662104 0:32.64 0

java_simple_serial.csv

Commit CPU Memory Time ExitCode
6e0d83f 179% 351068 0:13.29 0
c532320 170% 343216 0:14.42 0
c93f7af 163% 341344 0:15.11 0
d5b8616 169% 345504 0:15.08 0

java_gradle_multithreaded.csv

Commit CPU Memory Time ExitCode
6e0d83f 180% 376804 0:18.33 0
c532320 182% 355644 0:20.04 0
c93f7af 179% 558492 0:33.83 0
d5b8616 181% 647956 0:31.43 0

java_maven_serial.csv

Commit CPU Memory Time ExitCode
6e0d83f 181% 318024 0:17.65 0
c532320 181% 338348 0:18.11 0
c93f7af 147% 296200 0:30.25 0
d5b8616 168% 352072 0:28.27 0

java_maven_parallel.csv

Commit CPU Memory Time ExitCode
6e0d83f 184% 318832 0:21.98 0
c532320 184% 340820 0:20.92 0
c93f7af 158% 341744 0:31.93 0
d5b8616 174% 338508 0:26.05 0

java_gradle_serial.csv

Commit CPU Memory Time ExitCode
6e0d83f 179% 388428 0:21.85 0
c532320 177% 349640 0:21.11 0
c93f7af 166% 619168 0:33.39 0
d5b8616 179% 608220 0:36.92 0

Notes

  • The results above are generated by running against seed projects in https://github.com/getgauge/gauge-benchmark
  • These results are not persisted, but on merging to master the benchmark will be rerun.
  • These benchmark are run in Github Actions' agents, which are virtualized. Results are not to be taken as actual benchmarks.Rather, these are indicative numbers and make sense for comparison.

See Workflow log for more details.

@waldemar-kindler waldemar-kindler force-pushed the apple-silicon branch 2 times, most recently from 825e146 to fb72643 Compare May 20, 2021 13:36
@waldemar-kindler
Copy link
Contributor Author

hi @Bonitas

Would you prefer me to rebase sll commits into one for the DCO? Otherwise I think I'll need to edit the merge commit. Thats the one where I missed the sign.

Yes, a rebase should be fine in this case.

Also maybe it's worth to add arm support for linux

Yes, the change should be quite small, but it's the testing that I am worried about too. Let's defer this until someone finds a need for it.

Okay rebase is done.

sriv
sriv previously approved these changes May 20, 2021
@gaugebot
Copy link

gaugebot bot commented May 20, 2021

@Bonitas Thank you for contributing to gauge. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@sriv
Copy link
Member

sriv commented May 21, 2021

hi @Bonitas - I was hoping to make a release with this change. In order to do so, gauge version needs to be bumped up. I think 1.2.0 would be a good number.

Could you please set the version in version.go - see https://github.com/getgauge/gauge/blob/master/CONTRIBUTING.md#bump-up-gauge-version

waldemar-kindler and others added 3 commits May 21, 2021 10:02
Signed-off-by: Waldemar Kindler <waldemar.kindler@gmail.com>
Bumps [lodash](https://github.com/lodash/lodash) from 4.17.19 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.19...4.17.21)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.37.0 to 1.38.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.37.0...v1.38.0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@waldemar-kindler
Copy link
Contributor Author

hi @Bonitas - I was hoping to make a release with this change. In order to do so, gauge version needs to be bumped up. I think 1.2.0 would be a good number.

Could you please set the version in version.go - see https://github.com/getgauge/gauge/blob/master/CONTRIBUTING.md#bump-up-gauge-version

version is bumped. Thanks for your patience :)

@sriv sriv merged commit ebd763a into getgauge:master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants