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

Haskell#cabal_install more robust deps resolution #49158

Closed
wants to merge 1 commit into from

Conversation

ilovezfs
Copy link
Contributor

This could be made adjustable on a per formula basis, but since nothing in Language::Haskell::Cabal currently ensures only LTS configurations are used, always setting -1 (unlimited) makes sense.

@MikeMcQuaid
Copy link
Member

Can you link to an explanation of what this does? Thanks @ilovezfs!

@ilovezfs ilovezfs force-pushed the haskell-max-backjumps branch from 8f9eac0 to 8404a28 Compare February 14, 2016 11:18
@ilovezfs ilovezfs changed the title Don't restrict the number of backjumps that Haskell's caball_install method is allowed to use Don't restrict the number of backjumps that Haskell's cabal_install method is allowed to use Feb 14, 2016
@ilovezfs ilovezfs changed the title Don't restrict the number of backjumps that Haskell's cabal_install method is allowed to use Don't restrict the number of backjumps that Language::Haskell::Cabal's cabal_install method is allowed to use Feb 14, 2016
@ilovezfs ilovezfs force-pushed the haskell-max-backjumps branch from 8404a28 to efc729a Compare February 14, 2016 12:32
@DomT4
Copy link
Member

DomT4 commented Feb 14, 2016

Are there any downsides to this being the global default?

@ilovezfs
Copy link
Contributor Author

In general, I don't think so. The symptom would be that it takes "too long" or literally takes forever, but the build would timeout in that case during dependency resolution, so it's not like there's a mystery where the problem is.

The concern would be that this could potentially mask upstream issues where upstream really "should" change configuration to make it so that it can succeed with the default max-backjumps, which is currently 2000, but that probably goes in the saving-the-world-from-itself category, since Hackage will always be a nightmare regardless of what actions Homebrew takes.

It should be trivial to make it per formula now or later if global isn't the way you'd like to go.

@MikeMcQuaid
Copy link
Member

@ilovezfs Read those, still don't understand what a backjump is 😀. Can you elaborate in stupid-human speak?

However, a more robust way of installing Yesod is to use LTS Haskell, so that the dependency tree is solved and tested for you by the Stackage project.

Is this an option for us, somehow?

@ilovezfs
Copy link
Contributor Author

@MikeMcQuaid Wiki can do a better job than me, I think
https://en.wikipedia.org/wiki/Backjumping
https://en.wikipedia.org/wiki/Backtracking

Is this an option for us, somehow?

Yes, indeed.
https://www.fpcomplete.com/blog/2015/06/announcing-first-public-beta-stack
http://docs.haskellstack.org/en/stable/README.html
https://github.com/commercialhaskell/stack

Basically, brew would internally need to use the newfangled tool called "stack," which lets you do magical things like stack --resolver=lts-5.3, which causes it to use the consistent package set here:
https://www.stackage.org/lts-5.3

I also found a second solution to the git-annex build-failure, which is that passing --reorder-goals (instead of --max-backjumps=-1) allows it to succeed in forming its installation plan without running into the default backjump limit. In this particular case it also reduced the build time on this box from about 17 to 18 minutes down to about 14 to 15 minutes. However, in general, --reorder-goals has a negative influence on the performance of dependency resolution. They considered making it the default at one point, but rejected it due to the benchmarking results: haskell/cabal#1780

@MikeMcQuaid
Copy link
Member

@ilovezfs Great, thanks. I think I'm 🆗 with this solution as-is but it would be great to try and use this stack thing for more stability in future 👍

@UniqMartin
Copy link
Contributor

The symptom would be that it takes "too long" or literally takes forever, but the build would timeout in that case during dependency resolution, so it's not like there's a mystery where the problem is.

Does that mean that a user building locally from source could potentially end up with a stuck build that gives no indication on why it won't terminate? (Or are you referring to some cabal-specific timeout that would terminate the build nonetheless?)

@ilovezfs
Copy link
Contributor Author

There are cases where the algorithm will never stop.

@MikeMcQuaid
Copy link
Member

Could we maybe set the number to something very large instead?

@ilovezfs
Copy link
Contributor Author

Could we maybe set the number to something very large instead?

@MikeMcQuaid The default is 2000. In the case of the current git-annex failure, it required 43,478. So maybe set it to that, and readjust it up in the future if needed?

@ilovezfs
Copy link
Contributor Author

@MikeMcQuaid Another option is to do something like this:

      def cabal_install(*args)
        strategies ||= [ nil, "--reorder-goals", "--max-backjumps=100000" ]
        strategy = strategies.shift
        args << strategy unless strategy.nil?
        system "cabal", "install", "--jobs=#{ENV.make_jobs}", *args
      rescue RuntimeError
        raise if strategies.empty?
        retry
      end

I think it'd have to use popen if we wanted to match specifically for the backjumps error, and only retry on that.

@MikeMcQuaid
Copy link
Member

@ilovezfs Bumping it to 100,000 seems reasonable to me.

@ilovezfs
Copy link
Contributor Author

@MikeMcQuaid Does a magical number like that belong inline, in Library/Homebrew/config.rb, or somewhere else?

@MikeMcQuaid
Copy link
Member

@ilovezfs Inline is fine; just add a wee comment justifying it.

@ilovezfs ilovezfs force-pushed the haskell-max-backjumps branch from efc729a to d238841 Compare February 16, 2016 15:50
@ilovezfs
Copy link
Contributor Author

@MikeMcQuaid I refreshed the PR with 10^5 and a comment.

... a wee comment ...

Scottish shibboleth? :)

@ilovezfs ilovezfs force-pushed the haskell-max-backjumps branch from d238841 to ca351ac Compare February 16, 2016 15:56
@UniqMartin
Copy link
Contributor

@ilovezfs Thanks for adjusting that to a higher but still finite number of backjumps! If that solves the observed issue, I'm happy with this fix. 👍

@ilovezfs ilovezfs force-pushed the haskell-max-backjumps branch from ca351ac to 6d9f5dd Compare February 16, 2016 15:58
@ilovezfs
Copy link
Contributor Author

@UniqMartin Your wish is my command!

@UniqMartin
Copy link
Contributor

A few notes for this and/or future submissions (as you're a prolific contributor by now):

  • You're not doing Travis or our own CI any favor by pushing several updates to your branch in rapid succession. Maybe worth reviewing the changes locally before pushing them? (No hard feelings, just wanted to point that out in case you're not aware.)
  • You might want to adjust the commit subject/message to reflect the changes.
  • For a single-commit PR, it would help us if the commit subject and PR title would roughly match. Both could be a tad shorter (the goal for Git commit subjects is 50 characters) and (not directly applicable to this PR) have the form <formula>: fix <what was fixed>, as that makes the issue queue easier to scan and better aligns with our guidelines for commit subjects.

Thanks for considering these points. And thanks for the many good contributions in the recent past!

@ilovezfs
Copy link
Contributor Author

You're not doing Travis or our own CI any favor by pushing several updates to your branch in rapid succession. Maybe worth reviewing the changes locally before pushing them? (No hard feelings, just wanted to point that out in case you're not aware.)

My apologies. Of course I review them locally first. Some typos have a way of escaping my notice until I see the text formatted on GitHub for some reason. I assumed that the CI immediately canceled a prior job for the same PR.

if the commit subject and PR title would roughly match

I've deliberately been making the PR titles use more expansive and alternate language, and keeping the commit subject titles < 50 and more narrow. So thanks for piping up because that's not been an accident.

@ilovezfs ilovezfs changed the title Don't restrict the number of backjumps that Language::Haskell::Cabal's cabal_install method is allowed to use Haskell#cabal_install more robust deps resolution Feb 16, 2016
@ilovezfs
Copy link
Contributor Author

Isn't that policing too hard?

No. General-user security trumps power-user convenience every day and twice on Sunday. The primary FAQ for the haskell-stack formula is specifically guiding users to disable SIP, and they specifically point to Homebrew as their main means of distribution of the stack tool to Mac users. They need to get serious about being SIP compliant or accept full responsibility and distribute it themselves.

@MikeMcQuaid
Copy link
Member

No. General-user security trumps power-user convenience every day and twice on Sunday.

👏
I filed commercialhaskell/stack#1799. Let's see what happens there.

@ilovezfs
Copy link
Contributor Author

@MikeMcQuaid Perfect.

@zmwangx
Copy link
Contributor

zmwangx commented Feb 17, 2016

@ilovezfs

Homebrew also recommends messing with SIP, although it's quickly re-enabled: https://github.com/Homebrew/homebrew/blob/master/share/doc/homebrew/El_Capitan_and_Homebrew.md. You could argue that there's also general user security problem there — what if the user forget to re-enable it?

Also, as I said, they are just doing users (who do run into the edge case — hopefully not too many) a favor by mentioning it in the FAQ. Otherwise it's just one issue hanging open, and people will search about it and end up with the same solution. Except more time is wasted.

@MikeMcQuaid
Copy link
Member

@zmwangx Good point. @DomT4 I think we can remove that guide now that OS X is sufficiently updated that we never see people hitting that case any more.

@ilovezfs
Copy link
Contributor Author

@zmwangx I'm well aware:
Homebrew/install#27
#45387

@ilovezfs
Copy link
Contributor Author

@zmwangx Good point. @DomT4 I think we can remove that guide now that OS X is sufficiently updated that we never see people hitting that case any more.

@MikeMcQuaid That's a very good idea. People often still ask about it in IRC channel and think it's something they should/may need to do now/on update/at-some-point because of that FAQ.

@zmwangx
Copy link
Contributor

zmwangx commented Feb 17, 2016

Okay, objection attempt invalided I guess. Anyway, I still think it's way too paranoid. I agree that a formula should be rejected if it outright doesn't work with SIP on. But if it works 100% of the time for 95% of users, and requires a tweak 0.5% of the time for the remaining 5%, what's the point of making 95% people's lives harder? This is probably going to end in one of the following ways:

  1. stack removes that SIP entry from FAQ. People will still encounter the problem, will still search about it, and will end up with the same solution, just not endorsed. There are people with worse google-fu so maintainers will get more inquiries. Outcome: time wasted, maintainers annoyed.
  2. stack insists on keeping that entry in FAQ and Homebrew keeps the formula. Life goes on.
  3. stack insists on keeping that entry in FAQ, and the formula is rejected from Homebrew. Feelings hurt, people (mostly users who used to install stack via Homebrew) annoyed. Then stack is again awkwardly submitted to homebrew-cask (there used to be a stack cask, I happened to be the guy who removed it since it better belongs to Homebrew). But since homebrew-cask can't handle upgrades, no one's happy.

Which outcome would you like to see? I'd go for 2, or 1 at least.

@ilovezfs
Copy link
Contributor Author

@zmwangx None of the above. I'd like to see them remove the entry and fix any and all reliance on SIP ever being disabled by anyone at any time, whether due to DYLD_LIBRARY_PATH or any other reason. As for Homebrew Cask, it has the same standards with respect to SIP.

@MikeMcQuaid
Copy link
Member

Agreed with @ilovezfs. This is well within their power to change; it's effectively an OS X breaking API change that they are asking people to disable security features to work around.

@zmwangx
Copy link
Contributor

zmwangx commented Feb 17, 2016

@ilovezfs

I'd like to see them remove the entry and fix any and all reliance on SIP ever being disabled by anyone at any time, whether due to DYLD_LIBRARY_PATH or any other reason.

Except it's not going to happen easily. According to their FAQ,

The only workaround we are aware of is disabling System Integrity Protection.

That's basically saying "we need help or it'll stay that way". If you really care so much about SIP (I don't) and you know a fix, it's well within your power to submit a patch, as always.

Also, this is not an ideal world. Even Apple vendors binaries that only work when SIP is disabled, e.g., dtrace. As I've stated more than once, disabling or not is at the user's discretion. They did make it clear that

Note that this reduces the security of your system.

@MikeMcQuaid
Copy link
Member

That's basically saying "we need help or it'll stay that way". If you really care so much about SIP (I don't) and you know a fix, it's well within your power to submit a patch, as always.

It is. I already help Stack by maintaining Homebrew which maintains their software, though, and I don't have time to dig that deep on the implementation of a project I don't use, unfortunately.

@ilovezfs
Copy link
Contributor Author

If you really care so much about SIP (I don't)

EOF

@zmwangx
Copy link
Contributor

zmwangx commented Feb 17, 2016

I don't have time to dig that deep on the implementation of a project I don't use, unfortunately.

Yeah, but then the stack project also has more important goals than hack around runtime protections that only a tiny fraction of users may ever bump into. What's so wrong about the workaround "disable SIP if you are okay with it" to an issue in which no one wants to invest time?

@MikeMcQuaid
Copy link
Member

What's so wrong about the workaround "disable SIP if you are okay with it" to an issue in which no one wants to invest time?

I don't think Stack/Homebrew end-users are always in a good situation to weigh up the risks. If they need to use Stack and it isn't working and a guide says "follow these instructions to make it work": a lot of people (myself included) may just follow the steps without thinking too much about the consequences.

@zmwangx
Copy link
Contributor

zmwangx commented Feb 17, 2016

a lot of people (myself included) may just follow the steps without thinking too much about the consequences.

Yep, no doubt many people aren't interested in the details of SIP stuff and just want to have the issue at hand fixed. But are there actually consequences? Prior to El Capitan people lived peacefully with their Macs for many years... Many still do without SIP, and Homebrew does support those systems at least for now. It's not like you'll suddenly be pwned if you turn it off. To me, SIP is a nice-to-have, especially for technically-challenged general public without safe browsing habits, but policing packages that are slightly incompatible seems too much to me.

it's effectively an OS X breaking API change.

I think that's a good analogy. I'm sure there are many formulae in Homebrew that are slightly broken against OS X's APIs in certain edge cases, but they're included none the less because (1) they build; (2) they work at least most of the time. stack satisfies both criterions.

@ilovezfs
Copy link
Contributor Author

@zmwangx You're perfectly capable of researching the answer to that question yourself in a non-rhetorical fashion, and I'm sure your department has several relevant courses on computer security if it's a topic that interests you.

@MikeMcQuaid
Copy link
Member

This has been a good discussion but I think we've reached the end of it. Thanks for the input @ilovezfs and @zmwangx.

@DomT4
Copy link
Member

DomT4 commented Feb 17, 2016

@DomT4 I think we can remove that guide now that OS X is sufficiently updated that we never see people hitting that case any more.

Yup. I think that's fair. We can handle individual cases as we encounter them but Apple seems to have ironed out some of the earlier problems we had with SIP, and a lot of the people who were going to update to El Cap already have at this point. I'll remove it shortly.

This thread managed to triple the size of my GitHub inbox when I finally got around to checking it this afternoon, heh.

@DomT4
Copy link
Member

DomT4 commented Feb 17, 2016

@MikeMcQuaid
Copy link
Member

@DomT4 Thanks!

@MikeMcQuaid
Copy link
Member

They've compromised fairly well on this: commercialhaskell/stack#1799

@ilovezfs
Copy link
Contributor Author

@MikeMcQuaid @zmwangx FYI: #49387 making stack lts cabal.config an option

@MikeMcQuaid
Copy link
Member

Thanks again @ilovezfs!

@ilovezfs ilovezfs deleted the haskell-max-backjumps branch February 21, 2016 19:20
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants