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

get dwarf-fortress runnable: just install and type dwarf-fortress #21288

Merged
merged 12 commits into from
May 28, 2016
Merged

get dwarf-fortress runnable: just install and type dwarf-fortress #21288

merged 12 commits into from
May 28, 2016

Conversation

brewingcode
Copy link
Contributor

@brewingcode brewingcode commented May 18, 2016

  • Commit message includes cask’s name (and new version, if applicable).
  • brew cask audit --download {{cask_file}} is error-free.
  • brew cask style --fix {{cask_file}} left no offenses.

1. Replaced `suite` stanza with `binary`, DF doesn't belong in ~/Applications.

2. The DF binary requires launching from the directory it is in, so we
build a shimscript in preflight.

3. The binary's name is `df`, but this conflicts with the OSX `df` command,
so the symlink is `df_osx` instead.

4. Part of the installation on modern OSX requires replacing 2 of the
framework libraries that are included in the DF tarball. This is now
done automatically in postflight with a series of `system` calls.
@vitorgalvao
Copy link
Member

Downloading extra stuff via curl is a big no. Why is this better, and why is so much non-standard stuff needed for it?

@vitorgalvao vitorgalvao added the awaiting user reply Issue needs response from a user. label May 18, 2016
@brewingcode
Copy link
Contributor Author

brewingcode commented May 18, 2016

Darn, I figured extra curling was a big no, but figured I'd send the PR anyway. DF is ancient (14 years old) and only developed by a team of two, so the release format is...particular.

Is there some accepted method in homebrew-cask to supply a script that the user can optionally run after doing a cask install?

Edit: to answer the "why is this better" part, the previous cask install of DF resulted in a non-playable game, and no instructions on how to get it the rest of the way. Seeing as other cask installs result in immediately-playable games, I want the same for DF.

System calls are written to a shellscript instead, and the user needs to
run the script after installation on their own.
@brewingcode
Copy link
Contributor Author

brewingcode commented May 18, 2016

Removed the system calls, and instead diverted them to a shell script that the user will have to run on their own.

image

@adidalal
Copy link
Contributor

Might #20783 be of interest?

@brewingcode
Copy link
Contributor Author

I guess while I'm running around playing with casks involving DF, I can do #20783 as well, see #21290. :-)

I still want to get this change to dwarf-fortress in though, because it is just the base game, and it doesn't work as-is right now. In addition, the dwarf-fortress-lnp cask will move a little slower in terms of the version of DF that it includes. In fact, it's already behind:

cask DF version released
dwarf-fortress 43.01 2016-05-09
dwarf-fortress-lnp 42.04 2015-12-26

# http://dwarffortresswiki.org/index.php/DF2014:Installation#Mac
Utils.update_sdl 'SDL_ttf', 'projects/SDL_ttf/release/SDL_ttf-2.0.11.dmg', staged_path
Utils.update_sdl 'SDL', 'release/SDL-1.2.15.dmg', staged_path
ohai "Run #{staged_path}/update_sdl to fetch and replace DF's SDL libraries"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in a caveats block instead of ohai

@adidalal adidalal added awaiting user reply Issue needs response from a user. and removed awaiting user reply Issue needs response from a user. labels May 19, 2016
@vitorgalvao
Copy link
Member

vitorgalvao commented May 19, 2016

I still don’t like that you’re downloading and mountig external things (essentially doing what a cask is for) inside this cask. Can’t you push that to another cask and make this depend on the other?

Like we did Lightroom 6.3 depend on Lightroom 6.0.

@brewingcode
Copy link
Contributor Author

Can’t you push that to another cask and make this depend on the other?

Like we did Lightroom 6.3 depend on Lightroom 6.0.

Yep, I can do that. I was trying to avoid adding more casks, but if there's a precedent for it, I'm happy to follow it. Thanks for the link the Lightroom cask.

There will be two new casks I'll need, should each be their own PR, or can I lump them in here?

@adidalal
Copy link
Contributor

You can lump them - they are related and so one PR makes sense

There's still some File.rename and File.symlink to be done, but that's
not nearly as horrific as the previous mess.
Yes, putting use data alongside binaries is bad, but deleting it
outright is even worse.
@brewingcode brewingcode changed the title get dwarf-fortress runnable: just install and type df_osx get dwarf-fortress runnable: just install and type dwarf-fortress May 20, 2016
@adidalal
Copy link
Contributor

@brewingcode Not actively checking this PR but please comment here when changes are done so we can review and merge ASAP, thanks!

@brewingcode
Copy link
Contributor Author

I think the changes are done....? Let me know!

@adidalal
Copy link
Contributor

adidalal commented May 21, 2016

Casks look mechanically correct

@vitorgalvao if you could look through the shim script, that'd be appreciated.

@adidalal adidalal added awaiting maintainer feedback Issue needs response from a maintainer. and removed awaiting user reply Issue needs response from a user. labels May 21, 2016
depends_on cask: 'sdl-framework'
depends_on cask: 'sdl-ttf-framework'

binary shimscript, target: Hbc.homebrew_prefix.join('bin/dwarf-fortress')
Copy link
Member

Choose a reason for hiding this comment

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

binary shimscript, target: 'dwarf-fortress' should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but then the dwarf-fortress symlink ends up in /usr/local/bin, which doesn't work with Homebrews that are installed outside that default location.

Is there a reason homebrew-cask completely disregards that setting...? It seems like a bug: clearly homebrew-cask knows about custom prefixes because it already has the Hbc.homebrew_prefix property, so why doesn't it respect that property when symlinking binaries?

Copy link
Member

@vitorgalvao vitorgalvao May 23, 2016

Choose a reason for hiding this comment

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

but then the dwarf-fortress symlink ends up in /usr/local/bin

Not exactly. It ends up in your binarydir, which by default is /usr/local/bin, but may not be. That is a feature, not a bug, and makes sense in a world where HBC and HB are decoupled (currently the case).

We’re discussing changing that.

Currently, though, binary shimscript, target: 'dwarf-fortress' is then the correct option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I saw that binarydir command line option, and was surprised that it didn't take its default value from Homebrew's prefix.

OK, I'll change the cask, and maybe add a caveat about passing in --binarydir "$(brew --prefix)" or such during a cask install.

Copy link
Member

Choose a reason for hiding this comment

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

No need about adding that caveat, as this is not wrong behaviour, it is completely expected and documented.

@vitorgalvao vitorgalvao added awaiting user reply Issue needs response from a user. and removed awaiting maintainer feedback Issue needs response from a maintainer. labels May 22, 2016
@brewingcode
Copy link
Contributor Author

OK, changes pushed. Sorry for the delay, long week. :-)

@vitorgalvao vitorgalvao merged commit 4a4ae43 into Homebrew:master May 28, 2016
@vitorgalvao
Copy link
Member

Thank you for your continued work on this.

@brewingcode
Copy link
Contributor Author

My pleasure, thanks for pointing me in the right direction, and for merging.

chizmw pushed a commit to chizmw/homebrew-cask that referenced this pull request Jun 15, 2016
…omebrew#21288)

Also add casks: sdl-framework 1.2.15, sdl-ttf-framework 2.0.11, to support the main cask
@adidalal adidalal removed the awaiting user reply Issue needs response from a user. label Jul 2, 2016
@brewingcode brewingcode deleted the dwarf-fortress-sdl branch July 8, 2016 18:32
@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