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

WIP: Extract EULAs from a .dmg file #8231

Closed
wants to merge 1 commit into from
Closed

WIP: Extract EULAs from a .dmg file #8231

wants to merge 1 commit into from

Conversation

claui
Copy link
Contributor

@claui claui commented Dec 18, 2014

This is a first draft for extracting EULAs from a DMG.

I just wanted to try out the suggestions @phinze proposed and @MattiSG further elaborated in #720.

Please try this out

@federicobond @ndr-qef @phinze @rolandwalker @vitorgalvao

Despite this PR being little more than a proof-of-concept, I’d still love some feedback early on:

  • You can try out this branch right now. When you install one or more Casks from the above list, it should extract and display the EULA according to your locale.
  • Does the EULA show up for you?
  • I have used shell commands to get the prototype working quickly. Would you say it’s necessary that I rewrite part of this logic in Ruby? If so, which parts?

Testing

The following Casks are known to have their DMGs carry an EULA.
To help testing, please feel free to add more such Casks to this list.

To do

This is just a proof of concept so some more work is needed until this is ready to be reviewed/merged.

The main to-dos are:

  • Ask user to accept or decline the EULA
  • Give users auto-agree if they want auto-agree; give users their EULAs up front if they want the EULAs up front.
  • Find out whether we can rely on users having derez installed; alternatively, rewrite the derez part in Ruby
    Update: Switched to hdiutil udifderez which comes with OS X.
  • Port logic to Ruby where it makes sense
  • To improve UX, use less instead of just dumping the EULA; need to figure out how to persuade less that we are in a terminal
  • To improve UX, allow the user to actually press y or n instead of q; figure out how to make a custom .lesskey file
  • Write tests
  • Leverage RTF formatting, display bold text in color
  • Known issue: To extract the EULA, I unflatten the DMG but this causes the SHA2 of the DMG to change. Example where this causes subsequent installs to break: no-ip-duc Update: Fixed.
  • Review
  • Manual testing on different OS versions
  • Rigorous manual testing with as many Casks as possible
  • Remove most odebug statements

Implementation notes (WIP)

The implementation introduces two concepts:

  • in containers, a new eula? predicate, and
  • in the installer, an additional stage called display_eula.

This is what display_eula does:

  • Are we a DMG? If not, stop here.
  • Extract all EULAs from the primary container.
  • Decide if one of the locales match the user’s locale per the AppleLocale setting.
  • Fall back to the most common locales if we don’t have an exact match.
  • If we have a RTF, convert it to text.
  • Prefer RTF over text if there are both.

@claui claui added the core Issue with Homebrew itself rather than with a specific cask. label Dec 18, 2014
@vitorgalvao
Copy link
Member

Does the EULA show up for you?

It does not. Tried it with both mentioned casks on a mostly clean Yosemite VM, right after pulling the latest version of homebrew-cask, followed by a pull of this PR.

To improve UX, allow the user to actually press y or n instead of q; figure out how to make a custom .lesskey file

The reason that doesn’t strike me as a good idea in practice is y and n already have defined functions in less (back-line and repeat-search, respectively), and it’d be really easy to act on the agreement by mistake. Not that I think that’s a big issue in terms of the agreement itself, but I do think it’d make for a bad experience to have to repeat the command you just did on account of a single key press you might have done (possible a few times in a row) due to muscular memory.

:y (unset) and :n (next-file, which will never be needed in our context) look like acceptable solutions. However, both this and the single-key solution require pre-informing the user, which may be harder to do when they already have less open in front of them. Having to press q followed by the chosen option still seems like the best option, to me (less friction).

@claui
Copy link
Contributor Author

claui commented Dec 22, 2014

It does not.

@vitorgalvao Thanks a lot! Would you mind doing brew cask install with --debug?

When you look for something like this:

==> Checking container class Cask::Container::Dmg
==> Executing: ["/usr/bin/hdiutil", "imageinfo", "#<Pathname:/Library/Caches/Homebrew/no-ip-duc-3.2.1.dmg>"]
==> Using container class Cask::Container::Dmg for /Library/Caches/Homebrew/no-ip-duc-3.2.1.dmg

What does the output say immediately after those lines?

Thanks a lot for your help!

@vitorgalvao
Copy link
Member

Would you mind doing brew cask install with --debug?

Sure. I should’ve done it right away, completely forgot about the option; apologies.

With --debug, the EULA does show (same for both):

==> Checking container class Cask::Container::Dmg
==> Executing: ["/usr/bin/hdiutil", "imageinfo", "#<Pathname:/Library/Caches/Homebrew/no-ip-duc-3.2.1.dmg>"]
==> Using container class Cask::Container::Dmg for /Library/Caches/Homebrew/no-ip-duc-3.2.1.dmg
==> Executing: ["/bin/launchctl", "bsexec", "/", "/usr/bin/hdiutil", "mount", "-plist", "-nobrowse", "-readonly", "-noidme", "-mountrandom", "/tmp", "#<Pathname:/Library/Caches/Homebrew/no-ip-duc-3.2.1.dmg>"]
Warning: Non-XML stdout from launchctl:

  {{Full EULA shows here}}

@claui
Copy link
Contributor Author

claui commented Dec 22, 2014

Thanks a lot @vitorgalvao!

==> Executing: ["/bin/launchctl", "bsexec", "/", "/usr/bin/hdiutil", "mount", […]

This is really strange. There should be a lot of output before that line but your log doesn’t show any. I would really love to learn why yours doesn’t. I really need to be more generous with my debug statements :)

With --debug, the EULA does show (same for both)

Thanks a lot. The EULA dump you’re encountering is unrelated to the PR at hand. It is pre-existing behavior and happens at mount time. What the PR at hand is trying to accomplish is to work with any DMG, mounted or not.

@vitorgalvao
Copy link
Member

Like I said I’m doing the tests in a VM, so feel free to ask for any special settings, with complete disregard for what they could break.

@vitorgalvao
Copy link
Member

Any progress, @claui?

@vitorgalvao
Copy link
Member

Closed in favour of #13593.

@vitorgalvao vitorgalvao closed this Sep 4, 2015
@miccal miccal removed the core Issue with Homebrew itself rather than with a specific cask. label Dec 23, 2016
@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