-
Notifications
You must be signed in to change notification settings - Fork 204
Install shopify-extensions during gem installation #1496
Install shopify-extensions during gem installation #1496
Conversation
c6c0d00
to
5399ac6
Compare
f7c83d6
to
55ccc64
Compare
472bd42
to
9002909
Compare
STDERR.puts(error.message) | ||
rescue => error | ||
STDERR.puts("Unable to install shopify-extensions: #{error}") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rescue statements allow the installation of the binary to fail silently. This should eventually be changed to fail the installation and notify the user to file a bug report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job 👏 I'll top hat after lunch
fb7e815
to
1c41fd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in one of the comments, I'd avoid the interactions with GitHub API and build upon their conventional URLs instead.
MAKEFILE | ||
|
||
begin | ||
ShopifyExtensions.install( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have missed the discussion, but what's the motivation behind pulling the binary at installation time as opposed to vendoring it as part of releasing the package? Is it because we don't want to include the binaries of architectures other than ours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered Nokogiri's approach where multiple native gems are generated containing the right binary for each architecture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it because we don't want to include the binaries of architectures other than ours?
That is correct. We've considered this option but dismissed it due to binary size. The binaries are quite largue as each one includes the Go runtime: ~ 10 MB per binary (5 MB compressed)
Have we considered Nokogiri's approach where multiple native gems are generated containing the right binary for each architecture?
We have not. I wasn't aware that this option exists. From the link you provided, it's not immediately evident to me how to actually package and publish OS specific versions of Ruby gems. Do you have any experience with this yourself and could elaborate a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach we took follows the one that ESBuild uses to obtain its binary when you install the corresponding node package. Can you expand a bit on the concerns you have with this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pepibumur, do you see this as a blocker or something that we could improve incrementally. Since we're not releasing to partners before my intermission, I'd be ok to wait with integrating that, but it makes testing this a little more manual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion, we decided that this can be an incremental improvement and is no reason to hold back on this PR. Especially now that the installation of shopify-extensions
isn't done during gem installation for now. I'll investigate whether I want to pivot the approach when I'm back from intermission.
That said, now that we're no longer making API calls to GitHub but directly construct the download URL, I'm inclined to stick to the current approach.
Adds a new Ruby extension called `shopify-extensions` to download the development server during installation.
* Supports updating of shopify-extensions through shopify:extensions:update VERSION=vx.y.Z * Supports symlinking a local development build through shopify:extensions:symlink
4a1c5bc
to
abd7361
Compare
Removes API calls to Github in favor of constructing the download URL manually.
a8d12cb
to
7f224a5
Compare
7f224a5
to
0f98a82
Compare
The version is now stored in a plain text file instead of being part of the Ruby code. This makes updating the contents a little easier.
I removed running this code during gem installation for now. The code in this PR is therefore only affecting people who work on the CLI and only if they choose to run any of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tophat'd 👍
🎩'ed Windows one more time. |
WHY are these changes introduced?
This incorporates the new extension server that will be responsible for powering
WHAT is this pull request doing?
It introduces a new
InstallExtensionServer
operation that is run as part ofext/shopify-cli/extconf.rb
. The task is responsible for determining the appropriate binary version ofshopify-extensions
, download it, uncompress it and place it next to theshopify
bin stub.Tasks
Decompression and unpacking with Ruby appears to yield a different result than using
tar xf
. The MD5 checksum differs. It's crucial to set theb
flag when writing binary content to a file on Windows. The issue was therefore not related to TAR or GZip.↪ Yes, it is and it would therefore be easier to use the endpoint for fetching a single release:
https://api.github.com/repos/Shopify/shopify-cli-extensions/releases/tags/v0.1.0
zip
files totar.gz
files for windows releasesUpdate checklist
I've added a CHANGELOG entry for this PR (if the change is public-facing)The changes introduced in this PR are not yet Partner facing.🎩
rake extensions:install
ext/shopify-extensions/shopify-extensions version
yieldsv0.1.0
rake build && gem install pkg/shopify-cli-2.3.0.gem
(optionally verify that the installed version of the gem does not include the binary by usinggem open -e code shopify-cli
and double checking that the fileext/shopify-extensions/shopify-extensions
does not exist)