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

dbus: add manpages and missing build dependencies #6835

Closed
wants to merge 5 commits into from
Closed

dbus: add manpages and missing build dependencies #6835

wants to merge 5 commits into from

Conversation

zbentley
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

There are a few problems with the dbus formula:

  • This package has many scripts, all of which have manpages. However, those manpages aren't available in many standard (e.g. linux.die) websites, and are not compiled when this package is built. We should build the manpages. This requires some new build-time dependencies.
  • Building at HEAD (and possibly elsewhere) requires autoconf-archive, which is not listed as a dependency, and the build fails as a result.
  • --build-from-source is totally broken, since all of the build toolchain dependencies are inside a head block, preventing the toolchain from being available unless building at HEAD.
  • [Re]configure bootstrapping is disabled unless building at HEAD. This prevents documentation from building, and, aside from another ~6sec of build time (which isn't bad, considering what DBus is), is a totally unnecessary behavior fork. However, if there's good reason to leave this override in place, I am happy to undo this change.

Ordinarily I'd fix some of those in separate PRs, but they all block my ability to add the manpages (the real goal), so I'll fix 'em all in this PR.

@MikeMcQuaid
Copy link
Member

This requires some new build-time dependencies.

Which ones? If they are not usually built that makes me wonder if we should do so.

Building at HEAD (and possibly elsewhere) requires autoconf-archive, which is not listed as a dependency, and the build fails as a result.

It'll just be HEAD, please scope these changes just to HEAD unless you know for sure otherwise.

--build-from-source is totally broken, since all of the build toolchain dependencies are inside a head block, preventing the toolchain from being available unless building at HEAD.

That's not true, --build-from-source works fine as-is.

However, if there's good reason to leave this override in place, I am happy to undo this change.

We only want to reconfigure bootstrapping when it's necessary for some option or HEAD. Please keep it scoped to that.

@zbentley
Copy link
Contributor Author

Sorry for the confusion. At the top level there are two changes: adding the documentation builder, and removing the "skip bootstrap" build step, which is currently conditional on --HEAD. I can open a separate PR for the latter if that would help.

Which ones? If they are not usually built that makes me wonder if we should do so.

autoconf-archive is now required no matter what if building at --HEAD. With the old version of the formula, attempts to install at HEAD fail with the error in the attached
missingarchive.txt

We only want to reconfigure bootstrapping when it's necessary for some option or HEAD. Please keep it scoped to that.

I'm interested in why you say that, but that's totally separate from this PR. I will ship an update in the next ~15min that reduces changes down to just manpages and the HEAD dependency on autoconf-archive. Thanks!

100% aside from this PR: I think that it's best to have a single build process, and to avoid telling Homebrew to skip bootstrapping/use pre-built config/makefiles as a sort of "asset cache" if not building at HEAD. If build performance is the problem, or if the extra dependencies are considered bloat, there are bottles for that. Unless there is an important change in build dependencies between the sources of the default version and HEAD, I think it's best to keep the number of build paths as close to 1 as possible.

It is 100% possible/likely that I'm missing something dumb, though; I'm not very experienced at this.

depends_on "xmlto" => :build

# Docbook is a dependency of xmlto, but the XML_CATALOG_FILES env-set in
# doccbook is a dependency of xmlto, but the XML_CATALOG_FILES env-set in
Copy link
Contributor

Choose a reason for hiding this comment

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

one c


# Docbook is a dependency of xmlto, but the XML_CATALOG_FILES env-set in
# install() uses files created by docbook, so best to be explicit in case
# the install process for docbook ever stops creating those files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop this entire comment and the explicit docbook dependency. ascii, deheader, pgbouncer, reposurgeon, and shadowsocks-libev all have the same situation and none declares a docbook dependency.

@@ -36,13 +44,17 @@ class Dbus < Formula
def install
# Fix the TMPDIR to one D-Bus doesn't reject due to odd symbols
ENV["TMPDIR"] = "/tmp"

# Manpages won't build without a current docbook catalog. This should exist
Copy link
Contributor

@ilovezfs ilovezfs Nov 12, 2016

Choose a reason for hiding this comment

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

This comment is not needed. There are 29 cases just like this in core.

@@ -37,12 +45,16 @@ def install
# Fix the TMPDIR to one D-Bus doesn't reject due to odd symbols
ENV["TMPDIR"] = "/tmp"

# Manpages won't build without a current docbook catalog. This should exist
# if xmlto and docbook (build dependencies of this package) are installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not needed. There are 29 cases just like this in core.

@zbentley
Copy link
Contributor Author

I removed the comment, but I have to admit I can't see why I'm getting the version should not decrease error. Was there another update made? My Git-fu is not strong.

@MikeMcQuaid
Copy link
Member

Thanks again for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

version should not decrease

This was a bug and can be ignored.

@zbentley zbentley deleted the dbus-manpages branch November 13, 2016 19:37
@zbentley
Copy link
Contributor Author

Sorry for all the back-and-forth @MikeMcQuaid @ilovezfs; thanks!

@MikeMcQuaid
Copy link
Member

@zbentley No apologies needed, thanks!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 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