-
Notifications
You must be signed in to change notification settings - Fork 92
Use Rust FFI #2: Offload installers #279
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
Conversation
37b5c92 to
21267c1
Compare
| "config": { | ||
| "allow-plugins": { | ||
| "tienvx/composer-downloads-plugin": true | ||
| } |
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.
Add this config to make running test easier. I think each project depend on this library also need to define this config again in composer.json
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 will update README.md and/or CHANGELOG.md and/or UPGRADE-9.0.md about this change
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 think if we can provide support for a user to support the ffi being provided by them in their own location (which should be fine as we can have to provide a path to the library loader) we can support users who dont want to, or cant enable scripts to download.
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 agree with it.
This feature can be useful for plugins. See this link for explanation.
Currently I don't have any idea on how to do it. Can you create a ticket so we can look at it later?
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.
of course buddy 👍🏾 shall do now
| private static function getSuffix(): string | ||
| { | ||
| return $this->pactMessage; | ||
| return (PHP_OS_FAMILY === 'Windows' ? '.bat' : ''); |
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.
If we use symfony/process, I think we don't have to do this.
We can discuss this again in the future pull request (maybe the 4th pull request)
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 pull request for using symfony/process #284
|
|
||
| hub pull-request --browse --message "feat: update standalone to ${STANDALONE_VERSION}" | ||
|
|
||
| git checkout master |
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.
Now we only need to update 1 number in composer.json. That's why I think this script is not needed.
cfmack
left a comment
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.
Code looks good, I ran this locally and it passed, all tests in Github actions pass.
This is 2nd pull request in a series of "small" pull requests I split from #210
This pull request depend on #278, please review it first.
There are number of reasons why I choose this approach:
ziportar.gzfile is easy, but extractinggzipfile require more work, and we need to move the target file to the correct destination.*.gzdon't restore the original file name.composer install/updateAfter looking for alternate solution, I found this library civicrm/composer-downloads-plugin. But the author is also busy, he don't have time review my pull requests.
That's why I maintain my own fork. After making some changes, I released version 1.1, which is used by this pull request. This library has some features:
composer install/updatecommandI think this pull request also solve this ticket #276