-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Download platform/architecture specific binaries #33
Conversation
77e81b5
to
20b9cd6
Compare
Instead of rely on the package version I will create a separate binary tag / release and include it in the package.json.
|
82ff5d0
to
5a2a53e
Compare
index.js
Outdated
@@ -1,24 +1,22 @@ | |||
var os = require('os') | |||
var path = require('path') | |||
var os = require("os"); |
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.
Please revert these changes. They make it really hard to find out later when & how something was changed.
install.js
Outdated
var URL = require("url"); | ||
var pkg = require("./package"); | ||
|
||
function httpRequest(url, method, response) { |
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.
Use a slightly-higher-level lib like simple-get
for this. Unfortunately we'll still disrepect other important parts of the HTTP ecosystem, e.g. caching & proxy support.
I think that makes a lot of sense because it decouples ffmpeg releases from
The tests don't assert the functionality of the ffmpeg binaries anyways, so we don't need that. |
- just try on two for now
5a2a53e
to
4d09b24
Compare
d611fd4
to
188e119
Compare
ae446d4
to
27b9971
Compare
@derhuerst Sorry for the delay on wrapping this up I am taking a vacation this week and working on this when I can. The PR should be ready for review now, I made the changes requested. Overall I think this approach ended up being simpler than the multi-package approach. |
Note: The last commit tests are failing because that commit changes the download url to this repo, and there is not a binary release here yet. After this PR is merged you should create a You can also override it when you install the package with an environment variable ex |
I released this as @qawolf/ffmpeg-static and used it in qawolf to check it works. The tests pass and the install works properly. |
d1cde19
to
11b43e9
Compare
11b43e9
to
f692aff
Compare
Leaving the url as qawolf so tests pass. Will change it back before merging. I updated it to compress the binaries so the download size is much smaller. |
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.
Already looking very good!
Given that we only download one executable on install
, having the bin
structure and the excludes in package.json#files
feel overly complicated. Let's
- let
install.js
just download &index.js
just resolve to/ffmpeg
//ffmpeg.exe
, - let
build/index.sh
just put the executables inbin
(without subdirectories), with the same file names as in the GitHub release.
Thanks for your patience!
@derhuerst Thanks for the review 🙏. I made the changes you requested except #33 (comment) |
@derhuerst made the deltaBytes change. It should be good to go 👍 |
Good catches. Updated it 👍 |
@derhuerst to wrap this up
|
Resolves #20
Resolves https://github.com/qawolf/qawolf/issues/316
To Do
binary_release
for the version