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.

Sign up for GitHub

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

Jump to bottom

dbus: upstream Linux fixes #75915

Merged
merged 1 commit into from
Apr 27, 2021
Merged

dbus: upstream Linux fixes #75915

merged 1 commit into from
Apr 27, 2021

Conversation

danielnachun
Copy link
Member

  • 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?
  • Is your test running fine brew test <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>)?

@danielnachun danielnachun added the linux to homebrew-core Migration of linuxbrew-core to homebrew-core label Apr 26, 2021
@BrewTestBot BrewTestBot added the missing license Formula has a missing license which should be added label Apr 26, 2021
@danielnachun danielnachun mentioned this pull request Apr 26, 2021
Merged
5 tasks
@danielnachun
Copy link
Member Author

These failures all appear to be related to issues with qt, not this PR. If we think the changes look good, we should get this merged because other Linux-only formula depend on it.

@carlocab
Copy link
Member

@paperchalice is there a qt flag to prevent linkage with brotli?

@carlocab carlocab mentioned this pull request Apr 26, 2021
Closed
5 tasks
@carlocab
Copy link
Member

#75926

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Apr 26, 2021
@carlocab
qt: rebuild without brotli linkage
8b4bfe3 
qt picked up (opportunistic) linkage with `brotli` in Homebrew#74794.

See Homebrew#75915.
@iMichka
Copy link
Member

iMichka commented Apr 26, 2021

Wondering if we should disable the docs completely? We often do not ship docs. What was the error on Linux?

@paperchalice
Copy link
Contributor

paperchalice commented Apr 26, 2021 •
edited

@paperchalice is there a qt flag to prevent linkage with brotli?

Try -no-feature-brotli, module network may need it, qtwebengine in future may also need it.

@carlocab
Copy link
Member

@iMichka docs added in #6835. Seems like a user explicitly asked for them, and the docs are apparently not easily available online? This was a while ago, though, so things might be different now. If we can verify that the situation re doc availability elsewhere has changed, we can remove the docs.

@paperchalice already added the flag in #75926. Or should we just add the brotli dep, if it might actually be needed?

@danielnachun
Copy link
Member Author

Everything in the share/doc of the dbus cellar on macOS can be found here: https://dbus.freedesktop.org/doc/, along with much more. Also, there are no .xml files even present in the cellar, so --enable-xml-docs doesn't even seem to produce anything. If there are no objections, I'm just going to use --disable-xml-docs for both platforms.

@danielnachun
Copy link
Member Author

Scratch what I said above - --enable-xml-docs is needed to generate the man pages (I just tested this locally), which I think is something we like to provide to users if possible. IMO that wasn't a great choice of argument name by upstream.

I'm actually thinking instead that we may want to make xmlto a dependency on Linux as well and use --enable-xml-docs on both platforms. I'll have to test that on Linux to make sure it works.

@paperchalice
Copy link
Contributor

Adding depends_on "brotli" seems not bad, since brotli is a small package.

@carlocab
Copy link
Member

Merging #75926. You can rebase this when that's done.

BrewTestBot pushed a commit that referenced this pull request Apr 26, 2021
@carlocab @BrewTestBot
qt: rebuild without brotli linkage
fa5ff69 
qt picked up (opportunistic) linkage with `brotli` in #74794.

See #75915.

Closes #75926.

Signed-off-by: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com>
Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
carlocab
carlocab reviewed Apr 26, 2021
View reviewed changes
Formula/dbus.rb Outdated
Comment on lines 52 to 56
on_macos do
# macOS doesn't include a pkg-config file for expat
ENV["EXPAT_CFLAGS"] = "-I#{MacOS.sdk_path}/usr/include"
ENV["EXPAT_LIBS"] = "-lexpat"
end
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need this if you make pkg-config a build dependency on macOS too. I think taking out two on_os blocks makes the added dependency worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this locally and adding pkg-config as a build dependency for both platforms did allow me to remove those lines. This really cleans up the formula nicely!

@BrewTestBot BrewTestBot removed the missing license Formula has a missing license which should be added label Apr 27, 2021
@danielnachun
Copy link
Member Author

I added the missing license info and everything now passes in CI. Can I get a final review before merging this?

@danielnachun danielnachun merged commit 043700c into Homebrew:master Apr 27, 2021
@danielnachun danielnachun deleted the dbus branch May 1, 2021 00:32
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlocab carlocab carlocab approved these changes

Assignees
No one assigned
Labels
linux to homebrew-core Migration of linuxbrew-core to homebrew-core outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@danielnachun @carlocab @iMichka @paperchalice @BrewTestBot