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

audit: ensure default dependencies don't use options. #2482

Merged
merged 1 commit into from Apr 22, 2017

Conversation

Projects
None yet
7 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Apr 11, 2017

Default option dependencies are nasty as they cause unnecessary builds from source.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 11, 2017

This seems likely to encourage creation of new formula options. I don't think its scope should be confined to default dependencies.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Apr 12, 2017

Can this be optional for non-core taps? We have software that wants to use bullet with shared libraries (--with-shared) instead of static:

I definitely take your point about causing extra compilation from source, but from our perspective, it's necessary.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 17, 2017

I definitely take your point about causing extra compilation from source, but from our perspective, it's necessary.

Sorry, we don't believe in disabling audits like this for that. You could (should?) rework things so that that build from source isn't necessary i.e. by vendoring with a resource or adding a separate formula with the shared builds. Fundamentally Homebrew is attempting to transition from a from-source to a binary package manager and I'd recommend third-party taps seek the same.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 17, 2017

This seems likely to encourage creation of new formula options. I don't think its scope should be confined to default dependencies.

@ilovezfs Cool. This was was easier but will make that addition.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Apr 18, 2017

@MikeMcQuaid I hear you and respect your decisions, especially if it helps to reduce your maintenance and support workload. I haven't bothered to fork the bullet formula to change the default options, since it doesn't take very long to build. It would be different if it were something like qt. I certainly could make a bullet_shared.rb formula and build bottles for it, but then I also need to track upstream changes to bullet.rb, which is hard to automate and adds to my support workload.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 19, 2017

@scpeters it's not just about the time and support load. It's more so about the vulnerability of source builds to breaking at any time. If a binary install has dependencies that must build from source, it inherits that vulnerability.

For example, recently building netcdf --with-fortran spontaneously broke when Apple released Xcode 8.3 due to the addition of a new ld option named -lto_library, which CMake misinterpreted as referring to a library named libto_library.

This caused anything with depends_on "netcdf" => "with-fortran" not only to be unable to build but also to be unable to install at all even if it had a bottle.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 19, 2017

@scpeters regarding bullet specifically, it's likely the formula can be modified always to build and install both the static and shared libraries, and we could look at a PR for that if it's something you're interested in working on.

@scpeters scpeters referenced this pull request Apr 19, 2017

Closed

bullet: always build shared libraries #12636

4 of 4 tasks complete
@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Apr 19, 2017

@ilovezfs that's a good point too. I've submitted a PR to make bullet always build shared libraries, so that should help our use case: Homebrew/homebrew-core#12636

Thanks for your patience. I withdraw my objections to this PR.

@chdiza

This comment has been minimized.

Copy link
Contributor

chdiza commented Apr 19, 2017

Fundamentally Homebrew is attempting to transition from a from-source to a binary package manager

This is very, very sad.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 19, 2017

This is very, very sad.

If you looked at our analytics and see what percentages of users are using the default prefix and pouring bottles and also at the relative error rates on building from source vs. pouring bottles you'd be hard-pressed to justify why we should prioritise the experience of a small number of power users over the clear majority.

@MikeMcQuaid MikeMcQuaid reopened this Apr 19, 2017

audit: ensure dependencies don't use options.
Option dependencies are nasty as they cause unnecessary builds from
source.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:audit-dependency-options branch from 58595a0 to 3011cba Apr 21, 2017

@MikeMcQuaid MikeMcQuaid merged commit fdcffb2 into Homebrew:master Apr 22, 2017

1 of 3 checks passed

codecov/patch 40% of diff hit (target 64.07%)
Details
codecov/project 64% (-0.07%) compared to 6e1faf5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:audit-dependency-options branch Apr 22, 2017

@jonchang jonchang referenced this pull request May 1, 2017

Closed

Formulae with dependencies that require options #13133

47 of 47 tasks complete
@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented May 3, 2017

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented May 3, 2017

The big reason is that otherwise these will be resolved by moving things behind options.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented May 3, 2017

Your argument sounds to me like "options are bad", which goes a little farther than what I thought I heard from Mike, which was about avoiding builds from source for the vast majority of users who don't specify options on the command-line.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented May 3, 2017

Fundamentally Homebrew is attempting to transition from a from-source to a binary package manager and I'd recommend third-party taps seek the same.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented May 4, 2017

Ok, I guess I didn't read that properly the first time, but I'm not sure exactly what it means. I think of brew cask as a binary package manager, but homebrew formulae are instructions on how to build packages from source. Do you mean that brew install will only install bottles and you'll have to use brew test-bot or some dev-cmd to build bottles from source?

Is this driven by wanting to improve the user experience for your most frequent users and to reduce the maintenance burden?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 4, 2017

@scpeters brew cask is for distribution of upstream binaries. Homebrew is trying to distribute binaries rather than getting users to build from source whenever possible. This is because it saves time, saves upstream bandwidth and saves support requests (binaries are significantly easier to support).

We're never going to make it impossible to build formulae from source (after all, that's what we're doing ourselves) but we are going to keep ramping up brew audit such that it goes from pointing out problems to best practice for users.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented May 4, 2017

@MikeMcQuaid thanks for the clarification; that sounds reasonable.

@dakcarto

This comment has been minimized.

Copy link
Contributor

dakcarto commented May 12, 2017

@MikeMcQuaid wrote:

We're never going to make it impossible to build formulae from source (after all, that's what we're doing ourselves) but we are going to keep ramping up brew audit such that it goes from pointing out problems to best practice for users.

I think this certainly makes sense for formulae that have default dependencies with options (for the outlined reasons and bottling), but what about this scenario:

  ...
  option "with-api-docs", "Build the API documentation with Doxygen and Graphviz"
  ...
  if build.with? "api-docs"
    depends_on "graphviz" => :build
    depends_on "doxygen" => [:build, "with-graphviz"]
  end

Why would :build dependencies be required to have no options? Do I really have to vendor doxygen with graphviz support or create a new formula (for doxygen)? This seems quite restrictive for the build phase.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented May 12, 2017

To limit the vulnerability of the builds to random breakage in dependencies as already discussed above.

@dakcarto

This comment has been minimized.

Copy link
Contributor

dakcarto commented May 12, 2017

So, in my example above, I should separate out certain options to ancillary formulae, e.g. <formula-name>-api-doc? This would be similar to how Linux binary-only repos work.

Obviously, that works for things like docs, but I'm not so sure this won't cause more issues for intrinsic build options by forcing duplication to semi-redundant formulae (as @scpeters mentioned, before his specific problem was solved in a manner not generically applicable to all such situations).

In other words, formula authors are going to have to chose between what a given source code project considers optional (in their build configuration) and weighing that against defaulting to enabling more so that Homebrew users get the most out of their bottles. I believe this will many authors will just start turning on (or ask for) too many default build options for formula to compensate.

Just my 2-cents. Regardless, I do understand the reasoning behind this move.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented May 12, 2017

I think we're all well aware of the trade-offs here.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 12, 2017

In other words, formula authors are going to have to chose between what a given source code project considers optional (in their build configuration) and weighing that against defaulting to enabling more so that Homebrew users get the most out of their bottles. I believe this will many authors will just start turning on (or ask for) too many default build options for formula to compensate.

This is the inherent compromise in doing package management. This audit is just providing a gentle nudge to not punt this indecisiveness (and this includes mine) onto users in a way that's more time consuming and error-prone for them.

@jslee02 jslee02 referenced this pull request May 20, 2017

Closed

ode 0.15.2 #13761

4 of 4 tasks complete

@blogabe blogabe referenced this pull request May 22, 2017

Closed

xplanetFX 2.6.13 #13356

4 of 4 tasks complete
@muellermartin

This comment has been minimized.

Copy link

muellermartin commented May 27, 2017

@MikeMcQuaid regarding the analytics on the precentage of users pouring from bottles: Did you consider that power users who tend to use custom options also tend to disable analytics? This might distort your view of the usage.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 27, 2017

@muellermartin We will not be optimising the use of Homebrew for users who disable analytics or really "power users" but instead the 99% case.

@chdiza

This comment has been minimized.

Copy link
Contributor

chdiza commented May 27, 2017

We will not be optimising the use of Homebrew for users who disable analytics or really "power users" but instead the 99% case.

@muellermartin's point is it is not at all clear that they are the 99% use case.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 28, 2017

@muellermartin @chdiza Any my point is: if people opt-out they should not be surprised when they find we do not optimise for their usage of Homebrew.

@ilovezfs ilovezfs referenced this pull request Jul 8, 2017

Closed

Updated the CGAL formula to version 4.10 #15387

0 of 4 tasks complete

@ilovezfs ilovezfs referenced this pull request Aug 7, 2017

Closed

pymol: Patched in support for external gui in aqua #6066

8 of 8 tasks complete
@tjanson

This comment has been minimized.

Copy link

tjanson commented Aug 10, 2017

I found the message

Dependency #{$1} should not use option #{$2}

confusing, since it sounded like that particular option was deprecated. (I needed to look up the source and this PR to understand the intention of the warning.) Maybe a rewording like

Dependencies should not require any options

would be clearer.

@SConaway SConaway referenced this pull request Sep 7, 2017

Closed

OpenSSL seems to fail to build on 10.13db9 `17A360a` #17732

3 of 5 tasks complete

@charleshan5330 charleshan5330 referenced this pull request Oct 16, 2017

Closed

matplotlib: 2.1.0 #6382

8 of 8 tasks complete

@w4ltz3d w4ltz3d referenced this pull request Oct 18, 2017

Closed

Install javaHL failed #19584

@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.