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

[now-ruby] Use pre-installed ruby 2.5 #2991

Merged
merged 5 commits into from
Sep 12, 2019
Merged

[now-ruby] Use pre-installed ruby 2.5 #2991

merged 5 commits into from
Sep 12, 2019

Conversation

styfle
Copy link
Member

@styfle styfle commented Sep 10, 2019

This PR will improve performance of @now/ruby deployments since it will not need to install ruby each time. We already have ruby 2.5 in the build environment so this PR makes the builder aware of that path.

I am still installing the bundler to avoid the conflicting versions since ruby 2.6 is installed in the PATH.

Benchmark

  • 01-cowsay (11s)
  • 02-cowsay-vendored (30s)
  • 03-cowsay-proc (11s)
  • 04-cowsay-vendored-nested (28s)
  • 05-sinatra (14s)
  • 06-rails (74s)

cc @nathancahill

This also adds a polyfill for Node 10 to fix #2998

@styfle styfle requested a review from coetry as a code owner September 10, 2019 17:56
@codecov-io
Copy link

codecov-io commented Sep 10, 2019

Codecov Report

Merging #2991 into canary will decrease coverage by 2.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           canary    #2991      +/-   ##
==========================================
- Coverage   13.27%   11.24%   -2.03%     
==========================================
  Files         268      268              
  Lines       10329    10086     -243     
  Branches     1223     1313      +90     
==========================================
- Hits         1371     1134     -237     
+ Misses       8896     8861      -35     
- Partials       62       91      +29
Impacted Files Coverage Δ
src/util/output/create-output.ts 31.25% <0%> (-12.34%) ⬇️
src/util/dev/server.ts 61.21% <0%> (-5.69%) ⬇️
src/util/dev/yarn-installer.ts 85.71% <0%> (-5.03%) ⬇️
src/util/dev/builder.ts 73.07% <0%> (-4.35%) ⬇️
src/util/dev/router.ts 78.57% <0%> (-4.13%) ⬇️
src/util/get-files.ts 87.77% <0%> (-3.89%) ⬇️
src/util/dev/builder-cache.ts 77.27% <0%> (-3.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c944706...3b50843. Read the comment docs.

@styfle
Copy link
Member Author

styfle commented Sep 10, 2019

@nathancahill Oddly enough, this seemed to perform better in 376ffe0 than it did in cfa1d61 and tests pass for both.

Now that the builder will share the same ruby installation for all builds, does GEM_HOME matter either way?

@kodiakhq kodiakhq bot merged commit 5cabcb7 into canary Sep 12, 2019
@kodiakhq kodiakhq bot deleted the ruby-perf branch September 12, 2019 13:35
styfle added a commit that referenced this pull request Sep 17, 2019
* [now-ruby] Use pre-installed ruby 2.5

* Change GEM_HOME

* Add polyfill for Node 8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is Rails supported on Now?
3 participants