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

Move Cask core code to Homebrew #725

Merged
merged 2 commits into from Aug 19, 2016

Conversation

Projects
None yet
6 participants
@AnastasiaSulyagina
Copy link
Contributor

AnastasiaSulyagina commented Aug 16, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully run brew tests with your changes locally?

Moving Cask core code to Homebrew as part of #14384.

Change is splitted in 2 commits to be easier to review. Any advice on better splitting or anything else welcome.

cc @MikeMcQuaid @vitorgalvao @jawshooah @adidalal (bring other cask maintainers please)

@jawshooah

This comment has been minimized.

Copy link
Contributor

jawshooah commented Aug 16, 2016

Pinging @reitermarkus, @mwean, @fanquake, @victorpopkov, @alebcay, @claui, and @phinze for visibility.

@MikeMcQuaid MikeMcQuaid referenced this pull request Aug 17, 2016

Merged

Prepare for Homebrew Cask to be imported. #727

4 of 4 tasks complete
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

CC @Homebrew/maintainers as this is a big one.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

I've opened #727 to fix the test failures here as they are in our code making assumptions about Cask code that we need to decide if they are valid.

@MikeMcQuaid

This comment has been minimized.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

You can probably remove all the "under Ruby 2.1" tests and may need some test fixtures to not assume Caskroom/homebrew-cask is tapped (although we could compromise that and just tap it before the test). It's probably worth doing these in a separate PR before merging this or the respective Cask PR.

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 17, 2016

Seems that cask tap's path for test-bot is not HOMEBREW_LIBRARY.join("Taps", "caskroom", "homebrew-cask"). Will take a look

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

Seems that cask tap's path for test-bot is not HOMEBREW_LIBRARY.join("Taps", "caskroom", "homebrew-cask"). Will take a look

@AnastasiaSulyagina on the test-bot: it's not even tapped 😀. This is something we could address at a later point, though.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

This is something we could address at a later point, though.

Although let me know which you'd rather do.

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 17, 2016

I'd rather tap it before tests.
Why to address it at later point and not now?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

I'd rather tap it before tests.

🆒 I can open a PR for that.

Why to address it at later point and not now?

Basically because at some point we're going to need to remove that reliance on having the tap tapped in favour of making tests create the Casks they expect and I figure that doesn't need to necessarily block this PR being merged.

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 17, 2016

I can open a PR for that.

That would be nice :)

Basically because at some point we're going to need to remove that reliance on having the tap tapped in favour of making tests create the Casks they expect and I figure that doesn't need to necessarily block this PR being merged.

Thanks, got it.

@MikeMcQuaid MikeMcQuaid referenced this pull request Aug 17, 2016

Merged

test-bot: tap cask before cask-tests. #730

4 of 4 tasks complete
@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 17, 2016

Speaking of Taps, If possible, you might want to merge in the last two commits in Homebrew/homebrew-cask#23821. The only thing I'm missing is an elegant way to redefine Tap::TAP_DIRECTORY for our tests.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

@reitermarkus I think you could merge that in for now as there's already conflicts there until there's a 👍 on holding off merging core PRs.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 17, 2016

Ok, merged the three remaining core PRs. Holding off now until this move is complete.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Aug 17, 2016

Do we need stuff like the GitHub templates, CoC & other dotfiles from Cask here? Are they not remaining with the Cask tap?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 17, 2016

Do we need stuff like the GitHub templates, CoC & other dotfiles from Cask here? Are they not remaining with the Cask tap?

@DomT4 I'm in favour of moving basically everything for now and then removing them once they are over; it's too hard to review the diff for now.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Aug 17, 2016

Cool. That was exactly my thought, that removing the unnecessary text files might make the diff actually reviewable, heh, but whether that's done in pre or post doesn't matter much.

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 17, 2016

@jawshooah, are these tests needed? If yes, why should directories be undeletable and should I edit the paths somehow to fix it?

rspec ./spec/cask/macos_spec.rb:43 # OS::Mac says the user library directory is undeletable
rspec ./spec/cask/macos_spec.rb:31 # OS::Mac says the home directory is undeletable
@jawshooah

This comment has been minimized.

Copy link
Contributor

jawshooah commented Aug 17, 2016

Undeletable directories include system and user directories that should never be included in uninstall :delete or zap :delete. Those tests ensure that, were a Cask author to mistakenly include, for example, ~/Library in uninstall :delete, we would not actually delete that directory.

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 17, 2016

As I understood /Users/brew/Jenkins/pr-brew/version/el_capitan/home/ is Dir.home for test-bot. In macos.rb there is ~ string which actually should equal Dir.home. Am I wrong?

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 17, 2016

This would mean we have need to call sub(%r{^~(?=(/|$))}, Dir.home) everywhere we expect to receive a relative path instead of (or in addition to) expand_path.

@AnastasiaSulyagina AnastasiaSulyagina force-pushed the AnastasiaSulyagina:move-cask branch from 1c95ee6 to 69d2073 Aug 18, 2016

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 18, 2016

Hm. I thought that as it never was an issue before and appeared now in tests only, I can just replace ~ with Dir.home in Undeletables list. If it's unacceptable I'll fix it

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 18, 2016

Why can build pass in travis-ci/pr and fail in default?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 18, 2016

@AnastasiaSulyagina The filesystems may have different permissions.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 18, 2016

@AnastasiaSulyagina, could you rather cange https://github.com/AnastasiaSulyagina/brew/blob/move-cask/Library/Homebrew/cask/lib/hbc/macos.rb#L364 to .map { |x| Pathname(x.sub(%r{^~(?=(/|$))}, Dir.home)).expand_path } instead of changing the whole list, to keep it at little more readable?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 18, 2016

Mentioned to @AnastasiaSulyagina in Slack:

ENV["HOMEBREW_HOME"] = ENV["HOME"] = "#{Dir.pwd}/home"
mkdir_p ENV["HOME"]
is the issue; the test-bot overrides the $HOME variable to stop things being dumped in ~ and I'm guessing it's not getting expanded consistently as a result.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 18, 2016

@MikeMcQuaid, might be worth knowing what File.expand_path is using internally then to see if we can also override it globally in tests.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 18, 2016

@MikeMcQuaid MikeMcQuaid referenced this pull request Aug 18, 2016

Merged

Assume Ruby 2. #23889

@AnastasiaSulyagina AnastasiaSulyagina force-pushed the AnastasiaSulyagina:move-cask branch from 69d2073 to 0cb9d85 Aug 18, 2016

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 18, 2016

Rebased both PRs, fixed macos.rb

@AnastasiaSulyagina AnastasiaSulyagina force-pushed the AnastasiaSulyagina:move-cask branch from 0cb9d85 to 24ba616 Aug 18, 2016

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 19, 2016

I might have found the reason why some of the undeletable? tests are still failing.

irb> File.identical?('does-not-exist', 'does-not-exist')
=> false
irb> File.identical?('exists', 'exists')
=> true

Seems like File.identical? doesn't work on nonexistent paths.

@AnastasiaSulyagina, could you try changing https://github.com/AnastasiaSulyagina/brew/blob/move-cask/Library/Homebrew/cask/lib/hbc/macos.rb#L369-L375 to the following?

  def system_dir?(dir)
    SYSTEM_DIRS.include?(Pathname.new(dir).expand_path)
  end

  def undeletable?(dir)
    UNDELETABLE_DIRS.include?(Pathname.new(dir).expand_path)
  end

@AnastasiaSulyagina AnastasiaSulyagina force-pushed the AnastasiaSulyagina:move-cask branch from 24ba616 to 50f6214 Aug 19, 2016

@AnastasiaSulyagina

This comment has been minimized.

Copy link
Contributor

AnastasiaSulyagina commented Aug 19, 2016

1) Hbc::SystemCommand with a very long STDERR output returns without deadlocking
     Failure/Error: wait(10).for {
       RSpec::Wait::TimeoutError
     # ./spec/cask/system_command_spec.rb:136:in `block (3 levels) in <top (required)>'

It is not related to the PR, is it?

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 19, 2016

@AnastasiaSulyagina, could you bump https://github.com/AnastasiaSulyagina/brew/blob/move-cask/Library/Homebrew/cask/spec/cask/system_command_spec.rb#L136 to 15 seconds? I don't think there's actually a deadlock, as this only times out occasionally.

@AnastasiaSulyagina AnastasiaSulyagina force-pushed the AnastasiaSulyagina:move-cask branch from 50f6214 to a38a992 Aug 19, 2016

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 19, 2016

Finally! 🎉

AnastasiaSulyagina added some commits Aug 18, 2016

@AnastasiaSulyagina AnastasiaSulyagina force-pushed the AnastasiaSulyagina:move-cask branch from a38a992 to 092e279 Aug 19, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 19, 2016

Merging this and Homebrew/homebrew-cask#23852 should follow soon afterwards 🎉. Great work, @AnastasiaSulyagina!

@MikeMcQuaid MikeMcQuaid merged commit bd1ded9 into Homebrew:master Aug 19, 2016

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.007%) to 58.004%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.