Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

add shellcheck, a static analysis tool for shell scripts #21231

Closed
wants to merge 4 commits into from
Closed

add shellcheck, a static analysis tool for shell scripts #21231

wants to merge 4 commits into from

Conversation

mislav
Copy link
Sponsor Contributor

@mislav mislav commented Jul 15, 2013

The project doesn't seem to have a version, so this is dubbed v0.0.1 and is frozen to the git SHA where the current master branch is.

Problems with this formula's build process (seeking feedback):

  • cabal update - Necessary to run at least once after Cabal has been installed to fetch the list of available packages. However, this writes the cached files to ~/.cabal (i.e. outside of Homebrew's prefix).
  • cabal install ... - Also writes some files to ~/.cabal, even with --global flag enabled. This doesn't perform the checks whether the packages are already installed. Is it worth adding a Haskell package dependency checker to Homebrew for this?
  • Tons of things to install for just a 7.5MB resulting binary! "ghc" is ~700 MB on disk, and build time in total can be around 10-15 minutes. Maybe offer a precompiled binary instead? I'm not sure about the Homebrew process around that, or portability.

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 15, 2013

I don't know why Homebrew CI fails:

Preprocessing executable 'cabal' for cabal-install-1.16.0.2...
Distribution/Client/Types.hs:31:8:
    Could not find module `Network.URI'
    There are files missing in the `network-2.3.1.1' package,
    try running 'ghc-pkg check'.
    Use -v to see a list of the files searched for.

Worked for me without problem on OS X 10.8.4. I didn't have any Haskell development environment prior to doing brew install ghc and cabal-install.

@MikeMcQuaid
Copy link
Member

Yeh, I think this probably isn't going to work as long as it requires writing stuff to .cabal. Any way of overriding that somehow?

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 15, 2013

Probably but I have to figure it out first, or someone who knows Haskell packages can chime in. You can leave this open in the meantime. Also there is the matter of the mysterious CI failure.

Permission to implement a Haskell package checker?

depends_on :haskell => 'regex-posix'

@mistydemeo
Copy link
Member

If there isn't a stable release yet, we're not likely to accept this at this point. Can you ask the author about tagging a release?

@MikeMcQuaid
Copy link
Member

Haskell package version checker seems like a good idea.

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 15, 2013

@mistydemeo Alright, going to do that

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 15, 2013

Here is my compiled binary: http://up.mislav.net/QF2i/shellcheck [7.5 MB]

How about we distribute that instead? Not sure about the portability of this one; don't have a Snow Leopard to test on

@mistydemeo
Copy link
Member

How about we get the bots to bottle it instead, so that we a) get a binary tailored to multiple OSs, and b) users can build from source should they want it?

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 15, 2013

You had me at "How about we get the bots to"

@mistydemeo
Copy link
Member

Mike and I can do that part. :D Don't worry about it. The important part is just to have a formula that can build from source on our bots.

As a part of every build process the bots build every piece of software for Snow Leopard, Lion and Mountain Lion, and generate bottles, e.g. binary packages. We don't actually upload all of those binary packages for users at the moment, but we do for software that's inconvenient to build for whatever reason. Haskell stuff definitely fits there, as far as I'm concerned.

@MikeMcQuaid If we do start bottling Haskell packages there are other popular packages I'd like to see included too, so we don't require users to go through the whole rigorous rigamarole.

head 'https://github.com/koalaman/shellcheck.git'

depends_on 'ghc' => :build
depends_on 'cabal-install' => :recommended
Copy link
Member

Choose a reason for hiding this comment

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

I see you call cabal in the formula. Wouldn't that mean that this is mandatory, not just recommended?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

If the person either installed Cabal themselves elsewhere on the system, or has haskell-platform from Homebrew, they already have Cabal. So I've made it optional that you may say --witout-cabal-install (at least that's what I think I did with :recommended?)

Copy link
Member

Choose a reason for hiding this comment

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

Probably better in that case to have a CabalInstalled requirement that checks if one already exists, and with a default formula to install cabal-install otherwise. You can see a few requirements set up like that here: https://github.com/mxcl/homebrew/blob/master/Library/Homebrew/requirements.rb

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Yep, working on that since you've posted the initial comment :)

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 15, 2013

@mistydemeo Updated! Please have a look.

It now detects missing Haskell packages but won't install with Cabal by itself. Rather, like for RubyGems dependencies, it will fail with manual installation instructions:

shellcheck: Unsatisfied dependency: regex-compat
Homebrew does not provide Haskell dependencies; install with:
  cabal install --global regex-compat
shellcheck: Unsatisfied dependency: regex-posix
Homebrew does not provide Haskell dependencies; install with:
  cabal install --global regex-posix
Error: Unsatisifed requirements failed this build.

The user then has to run cabal update and cabal install --global regex-compat regex-posix to proceed. This will write cache files to their ~/.cabal directory but at least this formula isn't reponsible for that anymore.

@adamv
Copy link
Contributor

adamv commented Jul 17, 2013

May need to be rebased on master.

@adamv
Copy link
Contributor

adamv commented Jul 18, 2013

Can we get support for haskel deps as a separate pull request?

@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 19, 2013

Can we get support for haskel deps as a separate pull request?

Done. I don't understand why you make me do this when you could have just cherry picked the commits.

@adamv
Copy link
Contributor

adamv commented Jul 19, 2013

Because I am lazier than you.

Avoids initializing anything in the home directory
When detecting a missing package, it suggests that you install it with:

  cabal --install global <PACKAGE>

Example usage in formulae:

  depends_on 'parsec' => :haskell
Allows formulae to conditionally depend on "cabal-install" if `cabal`
executable is not present. Otherwise, `cabal` might have already been
provided from "haskell-platform" or by manual install.

Usage:

  depends_on :cabal
@mislav
Copy link
Sponsor Contributor Author

mislav commented Jul 24, 2013

I have updated shellcheck formula to reference the recently tagged version.

BTW, the author says:

If you're considering binary distribution, you can easily link it statically to prevent portability issues:

ghc -O9 -optl-static -optl-pthread --make shellcheck

@@ -46,6 +47,7 @@ def command_line
when :python3 then "pip3 install"
when :rbx then "rbx gem install"
when :ruby then "gem install"
when :haskell then "cabal install --global"
Copy link

Choose a reason for hiding this comment

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

why is there the --global flag
here? it is really really really bad practice to force global package installs with cabal.

@cartazio
Copy link

cartazio commented Aug 1, 2013

to contextualize: * DO NOT PUT --global* in any build script that is doing a cabal install

It forces the end users package db to have the associated versions installed globally.

which is really hard to fix when it creates conflicting dependencies in haskell land.

considering most folks who are installing haskell via brew rather than via a ghc.pkg or haskellplatform.pkg or the like will be new haskell users, and adding packages to the global ghc pkg db should not be done automagically. Its a really really bad idea.

@dysinger
Copy link
Contributor

dysinger commented Aug 1, 2013

--global is yucky

@cartazio
Copy link

cartazio commented Aug 1, 2013

see our additional remarks here: #21335

@jacknagel
Copy link
Contributor

Just to clarify, Homebrew wouldn't ever execute this command, it's output as part of an error message when the dependency is not satisfied.

I understand your point still applies, but I don't want there to be any confusion about what that string is there for.

@cartazio
Copy link

cartazio commented Aug 1, 2013

umm, WHY is --global there? there is never a good reason to have "cabal install --global" rather than "cabal install" in an error message. It should NEVER be there.

@cartazio
Copy link

cartazio commented Aug 1, 2013

we have not objections to "cabal install", its that "cabal install --global" is NEVER a good idea.
(though i'm VERY relieved to hear that its not something that will be executed.)

@cartazio
Copy link

cartazio commented Aug 1, 2013

on an unrelated note: a cabal update before a cabal install run will help with resolving packages that are needed.

@ghost
Copy link

ghost commented Aug 1, 2013

I think the right way to do this, which @dysinger suggested in #21335, is to create sandboxed builds of whatever haskell tools one wants to install. This can be done with cabal-dev: http://hackage.haskell.org/package/cabal-dev

Sandboxing would allow us to:

  1. avoid touching ~/.cabal which @MikeMcQuaid was concerned about
  2. avoid interfering with the user's existing or future haskell configuration
  3. avoid needing the user to run cabal update, caball install, etc. manually (potentially creating other problems)

It may take longer and use more space but I think it is a cleaner solution that requires less complication for the user and potentially less support headaches for homebrew and haskell devs. However, since this approach ought to work particularly well with bottling so those concerns may be moot anyway.

@ghost
Copy link

ghost commented Aug 1, 2013

Aside from the sandboxing issue, we ought to consider a general approach for handling Haskell packages people may want to install. Creating an individual formula for each one may be reasonable for now but it will mean a lot of duplication of effort. I'm thinking a better solution may be to create a 'brew cabal' tool like 'brew pip' and 'brew gem' and use that to query hackage and provide package availability and versioning information. Installing a package through 'brew cabal' could then use the sandbox approach to keep things nice and tidy.

Is there interest in this idea? Might take me awhile to get to it but I could try to give it a go.

@mislav
Copy link
Sponsor Contributor Author

mislav commented Aug 1, 2013

Please continue any Haskell packages/Cabal discussion in #21335.

cabal-dev looks interesting. Anyone is welcome to use it to make an alternative to my patch. @darinmorrison: a brew-cabal command might be useful to Haskell developers but not to our need here which is to simply compile an executable with necessary dependencies. We want to avoid users doing any work. You could start maintaining brew-cabal in your own git repo and people could brew tap your repo to get the command available.

@@ -31,6 +31,7 @@ def the_test
when :python3 then %W{/usr/bin/env python3 -c import\ #{@import_name}}
when :ruby then %W{/usr/bin/env ruby -rubygems -e require\ '#{@import_name}'}
when :rbx then %W{/usr/bin/env rbx -rubygems -e require\ '#{@import_name}'}
when :haskell then %{/usr/bin/env ghc-pkg list '#{@import_name}' --global --simple-output --names-only | grep .}
Copy link

Choose a reason for hiding this comment

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

this --global is really a bad idea

@adamv
Copy link
Contributor

adamv commented Oct 24, 2013

Hey everyone what's the status of including this particular piece of software?

@mislav
Copy link
Sponsor Contributor Author

mislav commented Oct 24, 2013

@adamv I was told that my approach is bad and the discussion went on in #21335. There is no PR yet from others that makes this better, to my knowledge. We were waiting on the release of the mystical cabal-dev.

Closing this since there is incentive to solve this in a better way. Ideally, the shellcheck formula would install pre-built binaries.

@mislav mislav closed this Oct 24, 2013
@cartazio
Copy link

its not cabal-dev, cabal-install 1.18,

the cabal-install 1.18 formulae is out.

the folks who volunteered to help are kinda busy dealing with the general Mavericks support issue and getting the GHC hotfix sorted out. Once the dust settles on that, they'll probably be happy to help. They're kinda busy rightnow

@MikeMcQuaid
Copy link
Member

@mislav it was solved in a non-general way in the cabal-install formula. I tried to adapt pandoc to do the same but ran against some upstream issues. It should work for shellcheck though. See my WIP here:
https://github.com/mikemcquaid/homebrew/commit/47312a

@MikeMcQuaid
Copy link
Member

@darinmorrison Sounds good. Will be good to be able to drop the apple-gcc42 dependency. I didn't think you'd beat Go to it, though ;)

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

Successfully merging this pull request may close these issues.

None yet

7 participants