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

xray: add plist #73393

Closed
wants to merge 1 commit into from
Closed

xray: add plist #73393

wants to merge 1 commit into from

Conversation

xiruizhao
Copy link
Contributor

@xiruizhao xiruizhao commented Mar 18, 2021

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@BrewTestBot BrewTestBot added the go Go use is a significant feature of the PR or issue label Mar 18, 2021
sinoban
sinoban previously approved these changes Mar 18, 2021
@kidonng
Copy link
Contributor

kidonng commented Mar 18, 2021

Please provide a default config file like v2ray.

@carlocab
Copy link
Member

It would also be good to bump the revision here.

Formula/xray.rb Outdated Show resolved Hide resolved
Formula/xray.rb Outdated Show resolved Hide resolved
Formula/xray.rb Outdated Show resolved Hide resolved
Formula/xray.rb Outdated Show resolved Hide resolved
chenrui333
chenrui333 previously approved these changes May 4, 2021
Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ Bottle publish failed.

@BrewTestBot BrewTestBot dismissed chenrui333’s stale review May 4, 2021 06:07

bottle publish failed

@carlocab
Copy link
Member

carlocab commented May 4, 2021

Needed rebasing to publish the bottle.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the new syntax for plists? Example:

plist_options manual: "php-fpm"
service do
run [opt_sbin/"php-fpm", "--nodaemonize"]
run_type :immediate
keep_alive true
error_log_path var/"log/php-fpm.log"
working_dir var
end

Also, please bump the revision (either add revision 1 or increment the revision right after license) so that users get the updated version of the formula when they do brew upgrade. Squash your changes with your first commit when you do this.

Thanks!

@carlocab carlocab requested a review from SMillerDev May 4, 2021 07:22
@xiruizhao xiruizhao requested a review from carlocab May 10, 2021 06:01
@carlocab
Copy link
Member

carlocab commented May 10, 2021

Seems like this needs Homebrew/brew#11355, and that has to land in a release tag first.

Formula/xray.rb Outdated Show resolved Hide resolved
@xiruizhao
Copy link
Contributor Author

Seems like this needs Homebrew/brew#11355, and that has to land in a release tag first.

@carlocab It's merged, could you re-review this PR?

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience here. Just one question. Feel free to ping me again when you're ready.

Formula/xray.rb Outdated Show resolved Hide resolved
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
@xiruizhao xiruizhao requested a review from carlocab May 18, 2021 14:02
Comment on lines 45 to 47
resource("geosite").stage do
pkgshare.install "dlc.dat" => "geosite.dat"
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resource("geosite").stage do
pkgshare.install "dlc.dat" => "geosite.dat"
end
pkgshare.install resource("geosite") => "geosite.dat"

Does this not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that. Let me check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error: An exception occurred within a child process:
  TypeError: no implicit conversion of Resource into String

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's fine. We can keep this one.

@carlocab
Copy link
Member

Thanks, @xiruizhao. GitHub Actions is suffering a partial outage at the moment, so we'll probably need to re-run CI later when it's back up.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Go use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants