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

changed the new install method for baiduinput #6243

Closed
wants to merge 1 commit into from
Closed

changed the new install method for baiduinput #6243

wants to merge 1 commit into from

Conversation

pan-long
Copy link
Contributor

changed the new install method for baiduinput

@vitorgalvao
Copy link
Member

If a manual install is now needed, you don’t need to point to the executable inside the app bundle. The app bundle itself should suffice.

@pan-long
Copy link
Contributor Author

But the pkg file is no longer avaliable in the installer, and it throws errors when I install it.

@vitorgalvao
Copy link
Member

Yes, I understand. What I mean is that instead of manual_installer '安装百度输入法.app/Contents/MacOS/安装百度输入法', it should just be manual_installer '安装百度输入法.app'.

@pan-long
Copy link
Contributor Author

Ok, I see. I haves modified the location back to the app bundle. Thank you!

@vitorgalvao
Copy link
Member

Thank you. Could you please squash your commits?

@pan-long
Copy link
Contributor Author

Ok, squashed :)

@vitorgalvao
Copy link
Member

Let me just check something first. @rolandwalker can we strike caskroom_only true from this one?

@rolandwalker
Copy link
Contributor

@vitorgalvao I believe we can, because (for instance) backblaze.rb does not use it.

Please do remove it from this Cask, for now at least. I've removed some undocumented hackery from the caskroom_only implementation, and am now attempting to remove caskroom_only itself from all Casks, to make sure there is no longer a technical need for it.

But on the larger question, I'm still unsure if caskroom_only is a useful bit of annotation, or a useless bit of clutter. Opinions welcome. Maybe the annotation is a good thing for clarifying Casks like node-webkit.rb.

Homebrew has a keg_only stanza, which is a bit more sophisticated because it takes an argument explaining why. It also generates a useful message at install-time.

If we do keep caskroom_only, we should at least enforce that it conflicts with other ordinary artifacts.

@vitorgalvao
Copy link
Member

My stance is that yes, caskroom_only should be kept, and that it should output a helpful message when installing a cask with it. It should also only be used in cased like the aforementioned node-webkit, i.e. cases where there is no stanza to act on the download anyway (when there are no app, pkg, manual_installer, or similar).

@rolandwalker
Copy link
Contributor

That's agreeable, though we would have to hoist manual_installer out of the opaque caveats block.

@pan-long thanks for the contribution! And sorry for highjacking the thread.

@pan-long
Copy link
Contributor Author

@rolandwalker it's ok and I like this kind of discussion as I can learn from others also. Thank you!

@vitorgalvao
Copy link
Member

we would have to hoist manual_installer out of the opaque caveats block.

That actually sounds good, to me.

@pan-long Could you please remove the caskroom_only true stanza?

@pan-long
Copy link
Contributor Author

@vitorgalvao ok, sure.

@pan-long
Copy link
Contributor Author

@vitorgalvao Sorry, I have mistakenly deleted the original branch. So I have created a new pull request for it. #6273

@vitorgalvao
Copy link
Member

I’ve merged the other one, so I’ll close this one. Thank you for the contribution, @pan-long.

@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants