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

manim: add linked indirect dependencies #171786

Merged
merged 2 commits into from
May 18, 2024
Merged

manim: add linked indirect dependencies #171786

merged 2 commits into from
May 18, 2024

Conversation

bevanjkay
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 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 HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Seen in: https://github.com/Homebrew/homebrew-core/actions/runs/9094160676?pr=171740

@bevanjkay bevanjkay added the CI-no-bottles Merge without publishing bottles label May 15, 2024
@github-actions github-actions bot added python Python use is a significant feature of the PR or issue ffmpeg FFMPEG use is a significant feature of the PR or issue labels May 15, 2024
@bevanjkay bevanjkay force-pushed the manim-deps branch 2 times, most recently from 04a8d0f to 3581b4d Compare May 15, 2024 14:19
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

There are still remaining indirect deps.

On Linux:

Indirect dependencies with linkage:
  fontconfig
  gcc
  glib
  jpeg-turbo
  libtiff
  openblas
  pango
  zlib

On Intel macOS:

Indirect dependencies with linkage:
  libxcb
  pango

I'd add these to all macOS, though, since they're already in the dep tree anyway.

@bevanjkay
Copy link
Member Author

The other ones showed extra Linux deps on the rebuild run too. I'll update it in a couple of hours.

@bevanjkay
Copy link
Member Author

@carlocab I have added the extras, put gcc as a dep for both because it showed for MacOS and Linux, so it is more likely an actual dependent?

@carlocab
Copy link
Member

Actually, the resources are a mess.

There are a bunch that duplicate formulae that are already declared as dependencies (e.g. scipy on ARM).

We should also just use the pillow formula instead of the resource, which I suspect will let you get rid of a bunch of these dependencies.

@p-linnane
Copy link
Member

Actually, the resources are a mess.

moderngl-window doesn't have an sdist on PyPI, so we can't use bump-python-resources. I figure that's why this is so messy.

@carlocab
Copy link
Member

moderngl-window doesn't have an sdist on PyPI, so we can't use bump-python-resources. I figure that's why this is so messy.

That might be another reason, but the problems about duplicating formulae dependencies aren't really fixed by bump-python-resources, so it would still be messy even if we could use it...

@carlocab
Copy link
Member

Tried fixing it up a bit. Hopefully it works. 🤞

@carlocab carlocab removed the CI-no-bottles Merge without publishing bottles label May 18, 2024
Formula/m/manim.rb Outdated Show resolved Hide resolved
@carlocab carlocab force-pushed the manim-deps branch 2 times, most recently from fc46a90 to fc7ca95 Compare May 18, 2024 09:34
@carlocab carlocab added the ready to merge PR can be merged once CI is green label May 18, 2024
on_macos do
depends_on "gettext"
depends_on "harfbuzz"
end

on_linux do
depends_on "cmake" => :build
Copy link
Member

Choose a reason for hiding this comment

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

Needing cmake here is kinda suspicious, ngl

Copy link
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label May 18, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue May 18, 2024
Merged via the queue into master with commit 4b0abf2 May 18, 2024
14 checks passed
@BrewTestBot BrewTestBot deleted the manim-deps branch May 18, 2024 16:10
@bevanjkay
Copy link
Member Author

bevanjkay commented May 18, 2024

Thanks for finishing this up @carlocab

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2024
@chenrui333
Copy link
Member

some followup PR, #177164

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. ffmpeg FFMPEG use is a significant feature of the PR or issue outdated PR was locked due to age python Python use is a significant feature of the PR or issue ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants